1. Patchew
  2. Xen
  3. View series

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

Jan Beulich posted 1 patch 1 week, 2 days ago
Failed in applying to current master (apply log)
[PATCH] x86/vPIC: register only one ELCR handler instance
Posted by Jan Beulich 1 week, 2 days ago 
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@suse.com>

--- 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;
+            vpic->elcr = data;
+        }
+        else
+        {
+            /* Reader should not see hardcoded level-triggered slave IR. */
+            data = vpic->elcr & vpic_elcr_mask(vpic);
+            if ( !shift )
+                *val = data;
+            else
+                *val |= data << shift;
+        }
+
+        ++vpic;
+        shift += 8;
+    } while ( --bytes );
 
     return X86EMUL_OKAY;
 }
@@ -470,8 +479,7 @@ void vpic_init(struct domain *d)
     register_portio_handler(d, 0x20, 2, vpic_intercept_pic_io);
     register_portio_handler(d, 0xa0, 2, vpic_intercept_pic_io);
 
-    register_portio_handler(d, 0x4d0, 1, vpic_intercept_elcr_io);
-    register_portio_handler(d, 0x4d1, 1, vpic_intercept_elcr_io);
+    register_portio_handler(d, 0x4d0, 2, vpic_intercept_elcr_io);
 }
 
 void vpic_irq_positive_edge(struct domain *d, int irq)
Re: [PATCH] x86/vPIC: register only one ELCR handler instance
Posted by Roger Pau Monné 6 days, 14 hours ago 
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@suse.com>
> 
> --- 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?

Thanks, Roger.
Re: [PATCH] x86/vPIC: register only one ELCR handler instance
Posted by Jan Beulich 5 days, 14 hours ago 
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@suse.com>
>>
>> --- 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
Re: [PATCH] x86/vPIC: register only one ELCR handler instance
Posted by Roger Pau Monné 5 days, 13 hours ago 
On Tue, May 30, 2023 at 10:48:02AM +0200, Jan Beulich wrote:
> 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@suse.com>
> >>
> >> --- 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.

Oh, I see, I should have paid more attention to the "Slave IR is
always level trig.", might have been helpful if this was noted as an
internal implementation detail.

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Patchew 2.1-devel-0870f84

© 2016 - 2023 Red Hat, Inc.