WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH 3/3] xen: implement unplug protocol in xen_platfo

To: Kevin Wolf <kwolf@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH 3/3] xen: implement unplug protocol in xen_platform
From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Date: Tue, 28 Jun 2011 12:27:24 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx Devel" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>, Markus, "Michael S. Tsirkin" <mst@xxxxxxxxxx>, "qemu-devel@xxxxxxxxxx Developers" <qemu-devel@xxxxxxxxxx>, Alexander Graf <agraf@xxxxxxx>, Anthony Liguori <anthony@xxxxxxxxxxxxx>, Anthony Perard <anthony.perard@xxxxxxxxxx>, Armbruster <armbru@xxxxxxxxxx>
Delivery-date: Tue, 28 Jun 2011 04:24:09 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4E08A5D3.8040400@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <alpine.DEB.2.00.1106161645230.12963@kaball-desktop> <1308240319-13949-3-git-send-email-stefano.stabellini@xxxxxxxxxxxxx> <7424DA76-FD02-479F-B72A-D0B6EBBDE7DF@xxxxxxx> <4DFF0C0A.8040505@xxxxxxxxxx> <alpine.DEB.2.00.1106231248470.12963@kaball-desktop> <4E083EB9.4030806@xxxxxxxxxx> <alpine.DEB.2.00.1106271555540.12963@kaball-desktop> <4E08A5D3.8040400@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Alpine 2.00 (DEB 1167 2008-08-23)
On Mon, 27 Jun 2011, Kevin Wolf wrote:
> Am 27.06.2011 17:34, schrieb Stefano Stabellini:
> > On Mon, 27 Jun 2011, Kevin Wolf wrote:
> >> hw/ide/pci.h is just as internal as internal.h is. And even if you
> >> managed to access the same things without any IDE header file, I still
> >> think it's not the right level of abstraction because it relies on the
> >> implementation details of IDE.
> >>
> >> Just this line: pci_ide->bus[di->bus].ifs[di->unit].bs = NULL; Does this
> >> really look right to you to do anywhere outside IDE?
> >>
> >> I'm basically looking for the same as Michael who wanted to have network
> >> unplug handled through qdev, just that the IDE code doesn't support
> >> unplug yet.
> > 
> > I understand.
> > 
> > I created pci_piix3_xen_ide_init and moved the unplug code to
> > hw/ide/piix.c so that we don't have any internal knowledge of IDE code
> > in xen_platform.c any more, see below.
> > 
> > One remaining problem is that the generic pci unplug function is not
> > what I need in this case so I have to setup my own; but I hope that in
> > general the changes are going in the right direction.
> 
> Looks much better to me now. :-)

great :)


> Just a few comments inline:
> 
> > diff --git a/hw/ide.h b/hw/ide.h
> > index 34d9394..a490cbb 100644
> > --- a/hw/ide.h
> > +++ b/hw/ide.h
> > @@ -13,6 +13,7 @@ ISADevice *isa_ide_init(int iobase, int iobase2, int 
> > isairq,
> >  /* ide-pci.c */
> >  void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
> >                           int secondary_ide_enabled);
> > +PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int 
> > devfn);
> >  PCIDevice *pci_piix3_ide_init(PCIBus *bus, DriveInfo **hd_table, int 
> > devfn);
> >  PCIDevice *pci_piix4_ide_init(PCIBus *bus, DriveInfo **hd_table, int 
> > devfn);
> >  void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn);
> > diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> > index c349644..8ae9ff0 100644
> > --- a/hw/ide/piix.c
> > +++ b/hw/ide/piix.c
> > @@ -167,6 +167,41 @@ static int pci_piix4_ide_initfn(PCIDevice *dev)
> >      return pci_piix_ide_initfn(d);
> >  }
> >  
> > +static int pci_piix3_xen_ide_unplug(DeviceState *dev)
> > +{
> > +    PCIDevice *pci_dev;
> > +    PCIIDEState *pci_ide;
> > +    DriveInfo *di;
> > +    int i = 0;
> > +
> > +    pci_dev = DO_UPCAST(PCIDevice, qdev, dev);
> > +    pci_ide = DO_UPCAST(PCIIDEState, dev, pci_dev);
> > +
> > +    for (; i < 3; i++) {
> > +        di = drive_get_by_index(IF_IDE, i); 
> > +        if (di != NULL && di->bdrv != NULL && !di->bdrv->removable) {
> > +            DeviceState *ds = bdrv_get_attached(di->bdrv);
> > +            if (ds)
> > +                bdrv_detach(di->bdrv, ds);
> 
> Missing braces

This time I run checkpatch.pl on it.


> 
> > +            bdrv_close(di->bdrv);
> > +            pci_ide->bus[di->bus].ifs[di->unit].bs = NULL;
> > +            drive_put_ref(di);
> > +        }
> > +    }
> > +    qdev_reset_all(&(pci_ide->dev.qdev));
> > +    return 0;
> > +}
> > +
> > +PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int 
> > devfn)
> > +{
> > +    PCIDevice *dev;
> > +
> > +    dev = pci_create_simple(bus, devfn, "piix3-ide");
> > +    dev->qdev.info->unplug = pci_piix3_xen_ide_unplug;
> 
> Doesn't this modify the piix3-ide definition? Shouldn't matter in
> practice because you can't have additional IDE controllers anyway, but
> maybe worth a comment stating this.
> 
> The other, probably cleaner way of doing it would be adding another
> PCIDeviceInfo for xen-ide.
 
I have added piix3-ide-xen, that doesn't set no_hotplug = 1, so now the
code should be more coherent.

I'll send the new version of the patch separately.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>