* [PATCH] x86/vPIC: register only one ELCR handler instance
@ 2023-05-26 7:35 Jan Beulich
2023-05-29 8:39 ` Roger Pau Monné
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2023-05-26 7:35 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
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 = ¤t->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)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/vPIC: register only one ELCR handler instance
2023-05-26 7:35 [PATCH] x86/vPIC: register only one ELCR handler instance Jan Beulich
@ 2023-05-29 8:39 ` Roger Pau Monné
2023-05-30 8:48 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Roger Pau Monné @ 2023-05-29 8:39 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu
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 = ¤t->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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/vPIC: register only one ELCR handler instance
2023-05-29 8:39 ` Roger Pau Monné
@ 2023-05-30 8:48 ` Jan Beulich
2023-05-30 9:08 ` Roger Pau Monné
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2023-05-30 8:48 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, Andrew Cooper, Wei Liu
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 = ¤t->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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] x86/vPIC: register only one ELCR handler instance
2023-05-30 8:48 ` Jan Beulich
@ 2023-05-30 9:08 ` Roger Pau Monné
0 siblings, 0 replies; 4+ messages in thread
From: Roger Pau Monné @ 2023-05-30 9:08 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu
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 = ¤t->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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-05-30 9:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-26 7:35 [PATCH] x86/vPIC: register only one ELCR handler instance Jan Beulich
2023-05-29 8:39 ` Roger Pau Monné
2023-05-30 8:48 ` Jan Beulich
2023-05-30 9:08 ` Roger Pau Monné
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.