Re: [PATCH] x86/vPIC: register only one ELCR handler instance

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.05.2023 10:39, Roger Pau Monné wrote:
> On Fri, May 26, 2023 at 09:35:04AM +0200, Jan Beulich wrote:
>> There's no point consuming two port-I/O slots. Even less so considering
>> that some real hardware permits both ports to be accessed in one go,
>> emulating of which requires there to be only a single instance.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/arch/x86/hvm/vpic.c
>> +++ b/xen/arch/x86/hvm/vpic.c
>> @@ -377,25 +377,34 @@ static int cf_check vpic_intercept_elcr_
>>      int dir, unsigned int port, unsigned int bytes, uint32_t *val)
>>  {
>>      struct hvm_hw_vpic *vpic;
>> -    uint32_t data;
>> +    unsigned int data, shift = 0;
>>  
>> -    BUG_ON(bytes != 1);
>> +    BUG_ON(bytes > 2 - (port & 1));
>>  
>>      vpic = &current->domain->arch.hvm.vpic[port & 1];
>>  
>> -    if ( dir == IOREQ_WRITE )
>> -    {
>> -        /* Some IRs are always edge trig. Slave IR is always level trig. */
>> -        data = *val & vpic_elcr_mask(vpic);
>> -        if ( vpic->is_master )
>> -            data |= 1 << 2;
>> -        vpic->elcr = data;
>> -    }
>> -    else
>> -    {
>> -        /* Reader should not see hardcoded level-triggered slave IR. */
>> -        *val = vpic->elcr & vpic_elcr_mask(vpic);
>> -    }
>> +    do {
>> +        if ( dir == IOREQ_WRITE )
>> +        {
>> +            /* Some IRs are always edge trig. Slave IR is always level 
>> trig. */
>> +            data = (*val >> shift) & vpic_elcr_mask(vpic);
>> +            if ( vpic->is_master )
>> +                data |= 1 << 2;
> 
> Not that you added this, but I'm confused.  The spec I'm reading
> explicitly states that bits 0:2 are reserved and must be 0.
> 
> Is this some quirk of the specific chipset we aim to emulate?

I don't think so. Note that upon reads the bit is masked out again.
Adding back further context, there's even a comment to this effect:

+        else
+        {
+            /* Reader should not see hardcoded level-triggered slave IR. */
+            data = vpic->elcr & vpic_elcr_mask(vpic);

The setting of the bit is solely for internal handling purposes,
aiui.

Jan