On 17.07.2023 12:51, Roger Pau Monné wrote:
> On Mon, Jul 17, 2023 at 11:31:57AM +0200, Jan Beulich wrote:
>> While the referenced commit came without any update to the public header
>> (which doesn't clarify how the upper address bits are used), the
>> intention looks to have been that bits 12..19 and 40..63 form the pIRQ.
>> Negative values simply make no sense, and pirq_info() also generally
>> wants invoking with an unsigned (and not just positive) value.
>>
>> Since the line was pointed out by Eclair, address Misra rule 7.2 at the
>> same time, by adding the missing U suffix.
>>
>> Fixes: 88fccdd11ca0 ("xen: event channel remapping for emulated MSIs")
>> Signed-off-by: Jan Beulich <[email protected]>
>
> Acked-by: Roger Pau Monné <[email protected]>
Thanks.
> I have a question below, but not related to the change here.
>
>>
>> --- a/xen/arch/x86/hvm/irq.c
>> +++ b/xen/arch/x86/hvm/irq.c
>> @@ -383,7 +383,7 @@ int hvm_inject_msi(struct domain *d, uin
>>
>> if ( !vector )
>> {
>> - int pirq = ((addr >> 32) & 0xffffff00) | dest;
>> + unsigned int pirq = ((addr >> 32) & 0xffffff00U) | dest;
>>
>> if ( pirq > 0 )
>
> I do wonder whether this check is also accurate, as pIRQ 0 could be a
> valid value. Should it instead use INVALID_PIRQ?
I think 0 is okay to use here, as the low IRQs (at least the 16 ISA
ones) are all 1:1 mapped to their "machine" (i.e. Xen's) IRQ numbers.
And IRQ0 is always the timer, never given to guests (not even to
Dom0).
If we wanted to use INVALID_PIRQ here, we'd first need to make this
part of the public interface.
> Since there's no specification or documentation how this is supposed
> to work it's hard to tell.
Indeed.
Jan
