[Qemu-devel] [PATCH] pci: improve w1c mask handling

[Qemu-devel] [PATCH] pci: improve w1c mask handling

[Michael S. Tsirkin]

- save/restore must not check w1c bits
  since they are in fact guest controlled
- clear w1c bits on reset

Note: for express there are different kinds of
reset, some leave part of config space alone.
We will likely need a sticky bit mask to implement this.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Isaku, does the below make sense?

 hw/pci.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index e50de14..1a14a0e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -141,7 +141,8 @@ static void pci_device_reset(PCIDevice *dev)
     pci_update_irq_status(dev);
     /* Clear all writeable bits */
     pci_word_test_and_clear_mask(dev->config + PCI_COMMAND,
-                                 pci_get_word(dev->wmask + PCI_COMMAND));
+                                 pci_get_word(dev->wmask + PCI_COMMAND) |
+                                 pci_get_word(dev->w1cmask + PCI_COMMAND));
     dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
     dev->config[PCI_INTERRUPT_LINE] = 0x0;
     for (r = 0; r < PCI_NUM_REGIONS; ++r) {
@@ -293,7 +294,8 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
 
     qemu_get_buffer(f, config, size);
     for (i = 0; i < size; ++i) {
-        if ((config[i] ^ s->config[i]) & s->cmask[i] & ~s->wmask[i]) {
+        if ((config[i] ^ s->config[i]) &
+            s->cmask[i] & ~s->wmask[i] & ~s->w1cmask) {
             qemu_free(config);
             return -EINVAL;
         }
-- 
1.7.3.2.91.g446ac

[Qemu-devel] Re: [PATCH] pci: improve w1c mask handling

[Isaku Yamahata]

On Wed, Oct 27, 2010 at 04:05:58PM +0200, Michael S. Tsirkin wrote:
> - save/restore must not check w1c bits
>   since they are in fact guest controlled
> - clear w1c bits on reset
> 
> Note: for express there are different kinds of
> reset, some leave part of config space alone.
> We will likely need a sticky bit mask to implement this.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> Isaku, does the below make sense?
> 
>  hw/pci.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index e50de14..1a14a0e 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -141,7 +141,8 @@ static void pci_device_reset(PCIDevice *dev)
>      pci_update_irq_status(dev);
>      /* Clear all writeable bits */
>      pci_word_test_and_clear_mask(dev->config + PCI_COMMAND,
> -                                 pci_get_word(dev->wmask + PCI_COMMAND));
> +                                 pci_get_word(dev->wmask + PCI_COMMAND) |
> +                                 pci_get_word(dev->w1cmask + PCI_COMMAND));
>      dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
>      dev->config[PCI_INTERRUPT_LINE] = 0x0;
>      for (r = 0; r < PCI_NUM_REGIONS; ++r) {
> @@ -293,7 +294,8 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
>  
>      qemu_get_buffer(f, config, size);
>      for (i = 0; i < size; ++i) {
> -        if ((config[i] ^ s->config[i]) & s->cmask[i] & ~s->wmask[i]) {
> +        if ((config[i] ^ s->config[i]) &
> +            s->cmask[i] & ~s->wmask[i] & ~s->w1cmask) {

[i] is needed for w1cmask.
~s->w1cmask[i]
           ^^^



>              qemu_free(config);
>              return -EINVAL;
>          }
> -- 
> 1.7.3.2.91.g446ac
> 

-- 
yamahata

[Qemu-devel] Re: [PATCH] pci: improve w1c mask handling

[Michael S. Tsirkin]

On Thu, Oct 28, 2010 at 11:02:48AM +0900, Isaku Yamahata wrote:
> On Wed, Oct 27, 2010 at 04:05:58PM +0200, Michael S. Tsirkin wrote:
> > - save/restore must not check w1c bits
> >   since they are in fact guest controlled
> > - clear w1c bits on reset
> > 
> > Note: for express there are different kinds of
> > reset, some leave part of config space alone.
> > We will likely need a sticky bit mask to implement this.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > 
> > Isaku, does the below make sense?
> > 
> >  hw/pci.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index e50de14..1a14a0e 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -141,7 +141,8 @@ static void pci_device_reset(PCIDevice *dev)
> >      pci_update_irq_status(dev);
> >      /* Clear all writeable bits */
> >      pci_word_test_and_clear_mask(dev->config + PCI_COMMAND,
> > -                                 pci_get_word(dev->wmask + PCI_COMMAND));
> > +                                 pci_get_word(dev->wmask + PCI_COMMAND) |
> > +                                 pci_get_word(dev->w1cmask + PCI_COMMAND));
> >      dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
> >      dev->config[PCI_INTERRUPT_LINE] = 0x0;
> >      for (r = 0; r < PCI_NUM_REGIONS; ++r) {
> > @@ -293,7 +294,8 @@ static int get_pci_config_device(QEMUFile *f, void *pv, size_t size)
> >  
> >      qemu_get_buffer(f, config, size);
> >      for (i = 0; i < size; ++i) {
> > -        if ((config[i] ^ s->config[i]) & s->cmask[i] & ~s->wmask[i]) {
> > +        if ((config[i] ^ s->config[i]) &
> > +            s->cmask[i] & ~s->wmask[i] & ~s->w1cmask) {
> 
> [i] is needed for w1cmask.
> ~s->w1cmask[i]
>            ^^^

Yes, I sent out a borken patch but it's correct on my branch.

> 
> 
> >              qemu_free(config);
> >              return -EINVAL;
> >          }
> > -- 
> > 1.7.3.2.91.g446ac
> > 
> 
> -- 
> yamahata