| Message ID | 09fc4c14-07e8-4e59-a23e-bb295125f25a@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/HVM: load state checking | expand |
On Tue, Nov 28, 2023 at 11:35:46AM +0100, Jan Beulich wrote: > Master bit 2 is treated specially: We force it set, but we don't expose > the bit being set to the guest. While right now the read and write > handling can easily use the fixed mask, the restore input checking that > is about to be put in place wants to use the inverted mask to prove that > no bits are unduly set. That will require master bit 2 to be set. Otoh > the read path requires the bit to be clear (the bit can have either > value for the use on the write path). Hence allow use sites control over > that bit. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > v3: New, split from larger patch. > --- > I'm certainly open to naming suggestions for the new macro parameter. > "mb2" can certainly be misleading as to Multiboot 2. Yet "master_bit_2" > it too long for my taste, not the least because of the macro then > needing to be split across lines. Let's leave it as mb2, I think given the context it is difficult to mislead this code as having anything to do with multiboot. > > --- a/xen/arch/x86/hvm/vpic.c > +++ b/xen/arch/x86/hvm/vpic.c > @@ -41,7 +41,7 @@ > #define vpic_lock(v) spin_lock(__vpic_lock(v)) > #define vpic_unlock(v) spin_unlock(__vpic_lock(v)) > #define vpic_is_locked(v) spin_is_locked(__vpic_lock(v)) > -#define vpic_elcr_mask(v) ((v)->is_master ? 0xf8 : 0xde) > +#define vpic_elcr_mask(v, mb2) ((v)->is_master ? 0xf8 | ((mb2) << 2) : 0xde) > > /* Return the highest priority found in mask. Return 8 if none. */ > #define VPIC_PRIO_NONE 8 > @@ -387,7 +387,7 @@ static int cf_check vpic_intercept_elcr_ > if ( dir == IOREQ_WRITE ) > { > /* Some IRs are always edge trig. Slave IR is always level trig. */ > - data = (*val >> shift) & vpic_elcr_mask(vpic); > + data = (*val >> shift) & vpic_elcr_mask(vpic, 1); Not that it matters much, but I think you could use vpic_elcr_mask(vpic, 0) to strictly keep the same behavior as before? > if ( vpic->is_master ) > data |= 1 << 2; Since the bit is forcefully set here anyway. Regardless: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On 05.12.2023 18:29, Roger Pau Monné wrote: > On Tue, Nov 28, 2023 at 11:35:46AM +0100, Jan Beulich wrote: >> @@ -387,7 +387,7 @@ static int cf_check vpic_intercept_elcr_ >> if ( dir == IOREQ_WRITE ) >> { >> /* Some IRs are always edge trig. Slave IR is always level trig. */ >> - data = (*val >> shift) & vpic_elcr_mask(vpic); >> + data = (*val >> shift) & vpic_elcr_mask(vpic, 1); > > Not that it matters much, but I think you could use > vpic_elcr_mask(vpic, 0) to strictly keep the same behavior as > before? Indeed, as also said in the description. Personally I view it as (slightly) more logical to not mask off ... >> if ( vpic->is_master ) >> data |= 1 << 2; > > Since the bit is forcefully set here anyway. ... and then set the bit, hence why I chose to go with 1. > Regardless: > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. Jan
--- a/xen/arch/x86/hvm/vpic.c +++ b/xen/arch/x86/hvm/vpic.c @@ -41,7 +41,7 @@ #define vpic_lock(v) spin_lock(__vpic_lock(v)) #define vpic_unlock(v) spin_unlock(__vpic_lock(v)) #define vpic_is_locked(v) spin_is_locked(__vpic_lock(v)) -#define vpic_elcr_mask(v) ((v)->is_master ? 0xf8 : 0xde) +#define vpic_elcr_mask(v, mb2) ((v)->is_master ? 0xf8 | ((mb2) << 2) : 0xde) /* Return the highest priority found in mask. Return 8 if none. */ #define VPIC_PRIO_NONE 8 @@ -387,7 +387,7 @@ static int cf_check vpic_intercept_elcr_ if ( dir == IOREQ_WRITE ) { /* Some IRs are always edge trig. Slave IR is always level trig. */ - data = (*val >> shift) & vpic_elcr_mask(vpic); + data = (*val >> shift) & vpic_elcr_mask(vpic, 1); if ( vpic->is_master ) data |= 1 << 2; vpic->elcr = data; @@ -395,7 +395,7 @@ static int cf_check vpic_intercept_elcr_ else { /* Reader should not see hardcoded level-triggered slave IR. */ - data = vpic->elcr & vpic_elcr_mask(vpic); + data = vpic->elcr & vpic_elcr_mask(vpic, 0); if ( !shift ) *val = data; else
Master bit 2 is treated specially: We force it set, but we don't expose the bit being set to the guest. While right now the read and write handling can easily use the fixed mask, the restore input checking that is about to be put in place wants to use the inverted mask to prove that no bits are unduly set. That will require master bit 2 to be set. Otoh the read path requires the bit to be clear (the bit can have either value for the use on the write path). Hence allow use sites control over that bit. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v3: New, split from larger patch. --- I'm certainly open to naming suggestions for the new macro parameter. "mb2" can certainly be misleading as to Multiboot 2. Yet "master_bit_2" it too long for my taste, not the least because of the macro then needing to be split across lines.