Message ID | 5556059F020000780007A8D0@mail.emea.novell.com |
---|---|
State | New |
Headers | show |
Ping? >>> On 15.05.15 at 14:41, <JBeulich@suse.com> wrote: > Expecting the ROM BAR to be written with an all ones value when sizing > the region is wrong - the low bit has another meaning (enable/disable) > and bits 1..10 are reserved. The PCI spec also mandates writing all > ones to just the address portion of the register. > > Use suitable constants also for initializing the ROM BAR register field > description. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/hw/xen/xen_pt.c > +++ b/hw/xen/xen_pt.c > @@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID > > /* check unused BAR register */ > index = xen_pt_bar_offset_to_index(addr); > - if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) && > + if ((index >= 0) && (val != 0) && > + (((index != PCI_ROM_SLOT) ? > + val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != > XEN_PT_BAR_ALLF) && > (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED)) { > XEN_PT_WARN(d, "Guest attempt to set address to unused Base Address > " > "Register. (addr: 0x%02x, len: %d)\n", addr, len); > --- a/hw/xen/xen_pt_config_init.c > +++ b/hw/xen/xen_pt_config_init.c > @@ -726,8 +726,8 @@ static XenPTRegInfo xen_pt_emu_reg_heade > .offset = PCI_ROM_ADDRESS, > .size = 4, > .init_val = 0x00000000, > - .ro_mask = 0x000007FE, > - .emu_mask = 0xFFFFF800, > + .ro_mask = ~PCI_ROM_ADDRESS_MASK & ~PCI_ROM_ADDRESS_ENABLE, > + .emu_mask = (uint32_t)PCI_ROM_ADDRESS_MASK, > .init = xen_pt_bar_reg_init, > .u.dw.read = xen_pt_long_reg_read, > .u.dw.write = xen_pt_exp_rom_bar_reg_write, > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On Fri, 15 May 2015, Jan Beulich wrote: > Expecting the ROM BAR to be written with an all ones value when sizing > the region is wrong - the low bit has another meaning (enable/disable) > and bits 1..10 are reserved. The PCI spec also mandates writing all > ones to just the address portion of the register. > > Use suitable constants also for initializing the ROM BAR register field > description. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/hw/xen/xen_pt.c > +++ b/hw/xen/xen_pt.c > @@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID > > /* check unused BAR register */ > index = xen_pt_bar_offset_to_index(addr); > - if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) && > + if ((index >= 0) && (val != 0) && > + (((index != PCI_ROM_SLOT) ? > + val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != XEN_PT_BAR_ALLF) && The change seems looks good and in line with the commit message. But this if statement looks like acrobatic circus to me now. > (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED)) { > XEN_PT_WARN(d, "Guest attempt to set address to unused Base Address " > "Register. (addr: 0x%02x, len: %d)\n", addr, len); > --- a/hw/xen/xen_pt_config_init.c > +++ b/hw/xen/xen_pt_config_init.c > @@ -726,8 +726,8 @@ static XenPTRegInfo xen_pt_emu_reg_heade > .offset = PCI_ROM_ADDRESS, > .size = 4, > .init_val = 0x00000000, > - .ro_mask = 0x000007FE, > - .emu_mask = 0xFFFFF800, > + .ro_mask = ~PCI_ROM_ADDRESS_MASK & ~PCI_ROM_ADDRESS_ENABLE, > + .emu_mask = (uint32_t)PCI_ROM_ADDRESS_MASK, There is no need for the explicit cast, is there? > .init = xen_pt_bar_reg_init, > .u.dw.read = xen_pt_long_reg_read, > .u.dw.write = xen_pt_exp_rom_bar_reg_write, > > >
>>> On 05.06.15 at 13:32, <stefano.stabellini@eu.citrix.com> wrote: >> --- a/hw/xen/xen_pt.c >> +++ b/hw/xen/xen_pt.c >> @@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID >> >> /* check unused BAR register */ >> index = xen_pt_bar_offset_to_index(addr); >> - if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) && >> + if ((index >= 0) && (val != 0) && >> + (((index != PCI_ROM_SLOT) ? >> + val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != XEN_PT_BAR_ALLF) && > > The change seems looks good and in line with the commit message. But > this if statement looks like acrobatic circus to me now. I think the alternative of splitting it up into multiple if()-s would not be any better readable. >> --- a/hw/xen/xen_pt_config_init.c >> +++ b/hw/xen/xen_pt_config_init.c >> @@ -726,8 +726,8 @@ static XenPTRegInfo xen_pt_emu_reg_heade >> .offset = PCI_ROM_ADDRESS, >> .size = 4, >> .init_val = 0x00000000, >> - .ro_mask = 0x000007FE, >> - .emu_mask = 0xFFFFF800, >> + .ro_mask = ~PCI_ROM_ADDRESS_MASK & ~PCI_ROM_ADDRESS_ENABLE, >> + .emu_mask = (uint32_t)PCI_ROM_ADDRESS_MASK, > > There is no need for the explicit cast, is there? There is - PCI_ROM_ADDRESS_MASK being wider than 32 bits the assignment could cause compiler warning otherwise (which I think I actually ran into. Jan
On Fri, 5 Jun 2015, Jan Beulich wrote: > >>> On 05.06.15 at 13:32, <stefano.stabellini@eu.citrix.com> wrote: > >> --- a/hw/xen/xen_pt.c > >> +++ b/hw/xen/xen_pt.c > >> @@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID > >> > >> /* check unused BAR register */ > >> index = xen_pt_bar_offset_to_index(addr); > >> - if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) && > >> + if ((index >= 0) && (val != 0) && > >> + (((index != PCI_ROM_SLOT) ? > >> + val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != XEN_PT_BAR_ALLF) && > > > > The change seems looks good and in line with the commit message. But > > this if statement looks like acrobatic circus to me now. > > I think the alternative of splitting it up into multiple if()-s would not > be any better readable. Would you be OK if I rewrote the statement as follows? if ((index >= 0) && (val != 0)) { uint32_t vu; if (index == PCI_ROM_SLOT) { vu = val | (uint32_t)~PCI_ROM_ADDRESS_MASK; } else { vu = val; } if ((vu != XEN_PT_BAR_ALLF) && (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED)) { XEN_PT_WARN(d, "Guest attempt to set address to unused Base Address " "Register. (addr: 0x%02x, len: %d)\n", addr, len); } }
>>> On 05.06.15 at 18:41, <stefano.stabellini@eu.citrix.com> wrote: > On Fri, 5 Jun 2015, Jan Beulich wrote: >> >>> On 05.06.15 at 13:32, <stefano.stabellini@eu.citrix.com> wrote: >> >> --- a/hw/xen/xen_pt.c >> >> +++ b/hw/xen/xen_pt.c >> >> @@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID >> >> >> >> /* check unused BAR register */ >> >> index = xen_pt_bar_offset_to_index(addr); >> >> - if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) && >> >> + if ((index >= 0) && (val != 0) && >> >> + (((index != PCI_ROM_SLOT) ? >> >> + val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != > XEN_PT_BAR_ALLF) && >> > >> > The change seems looks good and in line with the commit message. But >> > this if statement looks like acrobatic circus to me now. >> >> I think the alternative of splitting it up into multiple if()-s would not >> be any better readable. > > Would you be OK if I rewrote the statement as follows? > > if ((index >= 0) && (val != 0)) { > uint32_t vu; > > if (index == PCI_ROM_SLOT) { > vu = val | (uint32_t)~PCI_ROM_ADDRESS_MASK; > } else { > vu = val; > } > > if ((vu != XEN_PT_BAR_ALLF) && > (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED)) { > XEN_PT_WARN(d, "Guest attempt to set address to unused Base Address " > "Register. (addr: 0x%02x, len: %d)\n", addr, len); > } > } Actually I agree that this indeed looks better. If I were to make the change, I'd do if ((index >= 0) && (val != 0)) { uint32_t vu = val; if (index == PCI_ROM_SLOT) { vu |= (uint32_t)~PCI_ROM_ADDRESS_MASK; } if ((vu != XEN_PT_BAR_ALLF) && ... though. But if you're going to do the edit without wanting me to re-submit, I'll certainly leave this to you. Just let me know which way you prefer it to be handled. Jan
On Mon, 8 Jun 2015, Jan Beulich wrote: > >>> On 05.06.15 at 18:41, <stefano.stabellini@eu.citrix.com> wrote: > > On Fri, 5 Jun 2015, Jan Beulich wrote: > >> >>> On 05.06.15 at 13:32, <stefano.stabellini@eu.citrix.com> wrote: > >> >> --- a/hw/xen/xen_pt.c > >> >> +++ b/hw/xen/xen_pt.c > >> >> @@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID > >> >> > >> >> /* check unused BAR register */ > >> >> index = xen_pt_bar_offset_to_index(addr); > >> >> - if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) && > >> >> + if ((index >= 0) && (val != 0) && > >> >> + (((index != PCI_ROM_SLOT) ? > >> >> + val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != > > XEN_PT_BAR_ALLF) && > >> > > >> > The change seems looks good and in line with the commit message. But > >> > this if statement looks like acrobatic circus to me now. > >> > >> I think the alternative of splitting it up into multiple if()-s would not > >> be any better readable. > > > > Would you be OK if I rewrote the statement as follows? > > > > if ((index >= 0) && (val != 0)) { > > uint32_t vu; > > > > if (index == PCI_ROM_SLOT) { > > vu = val | (uint32_t)~PCI_ROM_ADDRESS_MASK; > > } else { > > vu = val; > > } > > > > if ((vu != XEN_PT_BAR_ALLF) && > > (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED)) { > > XEN_PT_WARN(d, "Guest attempt to set address to unused Base Address " > > "Register. (addr: 0x%02x, len: %d)\n", addr, len); > > } > > } > > Actually I agree that this indeed looks better. If I were to make the > change, I'd do > > if ((index >= 0) && (val != 0)) { > uint32_t vu = val; > > if (index == PCI_ROM_SLOT) { > vu |= (uint32_t)~PCI_ROM_ADDRESS_MASK; > } > > if ((vu != XEN_PT_BAR_ALLF) && > ... > > though. But if you're going to do the edit without wanting me to > re-submit, I'll certainly leave this to you. Just let me know which > way you prefer it to be handled. I prefer if you resubmit, thanks!
--- a/hw/xen/xen_pt.c +++ b/hw/xen/xen_pt.c @@ -248,7 +248,9 @@ static void xen_pt_pci_write_config(PCID /* check unused BAR register */ index = xen_pt_bar_offset_to_index(addr); - if ((index >= 0) && (val > 0 && val < XEN_PT_BAR_ALLF) && + if ((index >= 0) && (val != 0) && + (((index != PCI_ROM_SLOT) ? + val : (val | (uint32_t)~PCI_ROM_ADDRESS_MASK)) != XEN_PT_BAR_ALLF) && (s->bases[index].bar_flag == XEN_PT_BAR_FLAG_UNUSED)) { XEN_PT_WARN(d, "Guest attempt to set address to unused Base Address " "Register. (addr: 0x%02x, len: %d)\n", addr, len); --- a/hw/xen/xen_pt_config_init.c +++ b/hw/xen/xen_pt_config_init.c @@ -726,8 +726,8 @@ static XenPTRegInfo xen_pt_emu_reg_heade .offset = PCI_ROM_ADDRESS, .size = 4, .init_val = 0x00000000, - .ro_mask = 0x000007FE, - .emu_mask = 0xFFFFF800, + .ro_mask = ~PCI_ROM_ADDRESS_MASK & ~PCI_ROM_ADDRESS_ENABLE, + .emu_mask = (uint32_t)PCI_ROM_ADDRESS_MASK, .init = xen_pt_bar_reg_init, .u.dw.read = xen_pt_long_reg_read, .u.dw.write = xen_pt_exp_rom_bar_reg_write,
Expecting the ROM BAR to be written with an all ones value when sizing the region is wrong - the low bit has another meaning (enable/disable) and bits 1..10 are reserved. The PCI spec also mandates writing all ones to just the address portion of the register. Use suitable constants also for initializing the ROM BAR register field description. Signed-off-by: Jan Beulich <jbeulich@suse.com>