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 <[email protected]> > >> > >> --- 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é <[email protected]> Thanks, Roger.
