Re: [PATCH 2/4] hw/xen: update Xen PV NIC to XenDevice model

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

On Tue, 2023-10-24 at 15:47 +0100, Paul Durrant wrote:
> On 17/10/2023 19:25, David Woodhouse wrote:
> > +
> > +#define xen_pv_printf(a, n, ...) qemu_printf(__VA_ARGS__)
> 
> Why define this...

In the first place, just to make it build in the short term. Then I
forgot to clean it up before posting. In my tree this is all tracing
now.

> 
> > @@ -232,7 +258,7 @@ static ssize_t net_rx_packet(NetClientState *nc, const 
> > uint8_t *buf, size_t size
> >       RING_IDX rc, rp;
> >       void *page;
> >   
> > -    if (netdev->xendev.be_state != XenbusStateConnected) {
> > +    if (netdev->rx_ring.sring == NULL) {
> 
> Why not a straight swap for xen_device_backend_get_state()? Hard to see 
> whether there any hidden side effects of this change otherwise.

Could do, but what I *actually* cared about when looking at that check,
was whether the ring pointer was NULL. So I checked that explicitly.

It should be identical.

> > +static void xen_netdev_frontend_changed(XenDevice *xendev,
> > +                                       enum xenbus_state frontend_state,
> > +                                       Error **errp)
> >   {
> > -    struct XenNetDev *netdev = container_of(xendev, struct XenNetDev, 
> > xendev);
> > -    net_tx_packets(netdev);
> > -    qemu_flush_queued_packets(qemu_get_queue(netdev->nic));
> > +    ERRP_GUARD();
> > +    enum xenbus_state backend_state = xen_device_backend_get_state(xendev);
> > +
> > +    trace_xen_netdev_frontend_changed(xendev->name, frontend_state);
> > +
> > +    switch (frontend_state) {
> > +    case XenbusStateInitialised:
> 
> I don't think that's really a valid state for a network frontend. Linux 
> netback just ignores it.

Must we? I was thinking of making the ->frontend_changed() methods
optional and allowing backends to just provide ->connect() and
->disconnect() methods instead if they wanted to. Because we have three
identical ->frontend_changed() methods now...

Attachment: smime.p7s
Description: S/MIME cryptographic signature