Re: [Qemu-devel] [PATCH 4/9] s390x: Add I/O adapter registration.

[Cornelia Huck]

On Wed, 16 Apr 2014 13:25:49 +0200
Alexander Graf <address@hidden> wrote:

> 
> On 14.04.14 18:48, Cornelia Huck wrote:
> > Register an I/O adapter interrupt source for when virtio-ccw devices start
> > using adapter interrupts.
> >
> > Reviewed-by: Thomas Huth <address@hidden>
> > Signed-off-by: Cornelia Huck <address@hidden>
> > ---
> >   hw/intc/s390_flic.c   |   59 
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >   hw/s390x/css.c        |   51 ++++++++++++++++++++++++++++++++++++++++++
> >   hw/s390x/css.h        |    4 ++++
> >   hw/s390x/virtio-ccw.c |    4 ++++
> >   hw/s390x/virtio-ccw.h |    1 +
> >   target-s390x/cpu.h    |   33 +++++++++++++++++++++++++++
> >   6 files changed, 152 insertions(+)
> >
> > diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> > index b2ef3e3..c033c8a 100644
> > --- a/hw/intc/s390_flic.c
> > +++ b/hw/intc/s390_flic.c
> > @@ -21,6 +21,11 @@
> >   #define FLIC_FAILED (-1UL)
> >   #define FLIC_SAVEVM_VERSION 1
> >   
> > +static KVMS390FLICState *s390_get_flic(void)
> > +{
> > +    return KVM_S390_FLIC(object_resolve_path("/machine/s390-flic", NULL));
> > +}
> > +
> >   void s390_flic_init(void)
> >   {
> >       DeviceState *dev;
> > @@ -148,6 +153,60 @@ static int __get_all_irqs(KVMS390FLICState *flic,
> >       return r;
> >   }
> >   
> > +int kvm_s390_register_io_adapter(uint32_t id, uint8_t isc, bool swap,
> > +                                 bool is_maskable)
> > +{
> > +    struct kvm_s390_io_adapter adapter = {
> > +        .id = id,
> > +        .isc = isc,
> > +        .maskable = is_maskable,
> > +        .swap = swap,
> > +    };
> > +    KVMS390FLICState *flic = s390_get_flic();
> > +    int r, ret;
> > +    struct kvm_device_attr attr = {
> > +        .group = KVM_DEV_FLIC_ADAPTER_REGISTER,
> > +        .addr = (uint64_t)&adapter,
> > +    };
> > +
> > +    if (!flic) {
> > +        return -ENOSYS;
> > +    }
> > +    if (!kvm_check_extension(kvm_state, KVM_CAP_IRQ_ROUTING)) {
> > +        return -ENOSYS;
> > +    }
> > +
> > +    r = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
> > +
> > +    ret = r ? -errno : 0;
> > +    return ret;
> > +}
> > +
> > +int kvm_s390_io_adapter_map(uint32_t id, uint64_t map_addr, bool do_map)
> > +{
> > +    struct kvm_s390_io_adapter_req req = {
> > +        .id = id,
> > +        .type = do_map ? KVM_S390_IO_ADAPTER_MAP : 
> > KVM_S390_IO_ADAPTER_UNMAP,
> > +        .addr = map_addr,
> > +    };
> > +    KVMS390FLICState *flic = s390_get_flic();
> > +    struct kvm_device_attr attr = {
> > +        .group = KVM_DEV_FLIC_ADAPTER_MODIFY,
> > +        .addr = (uint64_t)&req,
> > +    };
> > +    int r;
> > +
> > +    if (!flic) {
> > +        return -ENOSYS;
> > +    }
> > +    if (!kvm_check_extension(kvm_state, KVM_CAP_IRQ_ROUTING)) {
> > +        return -ENOSYS;
> > +    }
> > +
> > +    r = ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr);
> > +    return r ? -errno : 0;
> > +}
> > +
> >   /**
> >    * kvm_flic_save - Save pending floating interrupts
> >    * @f: QEMUFile containing migration state
> > diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> > index 7074d2b..a6d173f 100644
> > --- a/hw/s390x/css.c
> > +++ b/hw/s390x/css.c
> > @@ -39,6 +39,13 @@ typedef struct CssImage {
> >       ChpInfo chpids[MAX_CHPID + 1];
> >   } CssImage;
> >   
> > +typedef struct IoAdapter {
> > +    uint32_t id;
> > +    uint8_t type;
> > +    uint8_t isc;
> > +    QTAILQ_ENTRY(IoAdapter) sibling;
> > +} IoAdapter;
> > +
> >   typedef struct ChannelSubSys {
> >       QTAILQ_HEAD(, CrwContainer) pending_crws;
> >       bool do_crw_mchk;
> > @@ -49,6 +56,7 @@ typedef struct ChannelSubSys {
> >       uint64_t chnmon_area;
> >       CssImage *css[MAX_CSSID + 1];
> >       uint8_t default_cssid;
> > +    QTAILQ_HEAD(, IoAdapter) io_adapters;
> >   } ChannelSubSys;
> >   
> >   static ChannelSubSys *channel_subsys;
> > @@ -69,6 +77,48 @@ int css_create_css_image(uint8_t cssid, bool 
> > default_image)
> >       return 0;
> >   }
> >   
> > +int css_register_io_adapter(uint8_t type, uint8_t isc, bool swap,
> > +                            bool maskable, uint32_t *id)
> > +{
> > +    IoAdapter *adapter;
> > +    bool found = false;
> > +    int ret;
> > +
> > +    *id = 0;
> > +    QTAILQ_FOREACH(adapter, &channel_subsys->io_adapters, sibling) {
> > +        if ((adapter->type == type) && (adapter->isc == isc)) {
> > +            *id = adapter->id;
> > +            found = true;
> > +            ret = 0;
> > +            break;
> > +        }
> > +        if (adapter->id >= *id) {
> > +            *id = adapter->id + 1;
> > +        }
> > +    }
> > +    if (found) {
> > +        goto out;
> > +    }
> > +    adapter = g_new0(IoAdapter, 1);
> > +    ret = s390_register_io_adapter(*id, isc, swap, maskable);
> > +    if (ret == -ENOSYS) {
> > +        /* Keep adapter even if we didn't register it anywhere. */
> > +        ret = 0;
> > +    }
> > +    if (ret == 0) {
> > +        adapter->id = *id;
> > +        adapter->isc = isc;
> > +        adapter->type = type;
> > +        QTAILQ_INSERT_TAIL(&channel_subsys->io_adapters, adapter, sibling);
> > +    } else {
> > +        g_free(adapter);
> > +        fprintf(stderr, "Unexpected error %d when registering adapter 
> > %d\n",
> > +                ret, *id);
> > +    }
> > +out:
> > +    return ret;
> > +}
> > +
> >   uint16_t css_build_subchannel_id(SubchDev *sch)
> >   {
> >       if (channel_subsys->max_cssid > 0) {
> > @@ -1239,6 +1289,7 @@ static void css_init(void)
> >       channel_subsys->do_crw_mchk = true;
> >       channel_subsys->crws_lost = false;
> >       channel_subsys->chnmon_active = false;
> > +    QTAILQ_INIT(&channel_subsys->io_adapters);
> >   }
> >   machine_init(css_init);
> >   
> > diff --git a/hw/s390x/css.h b/hw/s390x/css.h
> > index e9b4405..380e8e7 100644
> > --- a/hw/s390x/css.h
> > +++ b/hw/s390x/css.h
> > @@ -99,4 +99,8 @@ void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, 
> > uint16_t schid,
> >                              int hotplugged, int add);
> >   void css_generate_chp_crws(uint8_t cssid, uint8_t chpid);
> >   void css_adapter_interrupt(uint8_t isc);
> > +
> > +#define CSS_IO_ADAPTER_VIRTIO 1
> > +int css_register_io_adapter(uint8_t type, uint8_t isc, bool swap,
> > +                            bool maskable, uint32_t *id);
> >   #endif
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index 2bf0af8..1193682 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -522,6 +522,10 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >                   dev->thinint_isc = thinint->isc;
> >                   dev->ind_bit = thinint->ind_bit;
> >                   cpu_physical_memory_unmap(thinint, hw_len, 0, hw_len);
> > +                ret = css_register_io_adapter(CSS_IO_ADAPTER_VIRTIO,
> > +                                              dev->thinint_isc, true, 
> > false,
> > +                                              &dev->adapter_id);
> > +                assert(ret == 0);
> >                   sch->thinint_active = ((dev->indicators != 0) &&
> >                                          (dev->summary_indicator != 0));
> >                   sch->curr_status.scsw.count = ccw.count - len;
> > diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
> > index 4393e44..0b70b91 100644
> > --- a/hw/s390x/virtio-ccw.h
> > +++ b/hw/s390x/virtio-ccw.h
> > @@ -85,6 +85,7 @@ struct VirtioCcwDevice {
> >       bool ioeventfd_disabled;
> >       uint32_t flags;
> >       uint8_t thinint_isc;
> > +    uint32_t adapter_id;
> >       /* Guest provided values: */
> >       hwaddr indicators;
> >       hwaddr indicators2;
> > diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> > index f332d41..53391fd 100644
> > --- a/target-s390x/cpu.h
> > +++ b/target-s390x/cpu.h
> > @@ -1062,6 +1062,9 @@ void kvm_s390_enable_css_support(S390CPU *cpu);
> >   int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
> >                                       int vq, bool assign);
> >   int kvm_s390_cpu_restart(S390CPU *cpu);
> > +int kvm_s390_register_io_adapter(uint32_t id, uint8_t isc, bool swap,
> > +                                 bool maskable);
> > +int kvm_s390_io_adapter_map(uint32_t id, uint64_t map_addr, bool do_map);
> >   #else
> >   static inline void kvm_s390_io_interrupt(S390CPU *cpu,
> >                                           uint16_t subchannel_id,
> > @@ -1086,6 +1089,16 @@ static inline int kvm_s390_cpu_restart(S390CPU *cpu)
> >   {
> >       return -ENOSYS;
> >   }
> > +static inline int kvm_s390_register_io_adapter(uint32_t id, uint8_t isc,
> > +                                               bool swap, bool maskable)
> > +{
> > +    return -ENOSYS;
> > +}
> > +static inline int kvm_s390_io_adapter_map(uint32_t id, uint64_t map_addr,
> > +                                          bool do_map)
> > +{
> > +    return -ENOSYS;
> > +}
> >   #endif
> >   
> >   static inline int s390_cpu_restart(S390CPU *cpu)
> > @@ -1131,4 +1144,24 @@ static inline int 
> > s390_assign_subch_ioeventfd(EventNotifier *notifier,
> >       }
> >   }
> >   
> > +static inline int s390_register_io_adapter(uint32_t id, uint8_t isc, bool 
> > swap,
> > +                                           bool is_maskable)
> > +{
> > +    if (kvm_enabled()) {
> > +        return kvm_s390_register_io_adapter(id, isc, swap, is_maskable);
> 
> The differentiation whether we need to call a KVM flic function or a 
> non-KVM flic function should depend on the object that gets 
> instantiated, not on kvm_enabled().

But which flic (the kvm-flic or the hypothetic non-kvm-flic) we have
still depends on kvm_enabled(), no? This is really just the backend
call, as the common processing is already done in the respective css
function. I don't see what this would buy us, as the flic-getter would
depend on kvm or !kvm anyway and we'd made the code harder to follow
IMO.

> 
> 
> Alex
> 
> > +    } else {
> > +        return -ENOSYS;
> > +    }
> > +}
> > +
> > +static inline int s390_io_adapter_map(uint32_t id, uint64_t map_addr,
> > +                                      bool do_map)
> > +{
> > +    if (kvm_enabled()) {
> > +        return kvm_s390_io_adapter_map(id, map_addr, do_map);
> > +    } else {
> > +        return -ENOSYS;
> > +    }
> > +}
> > +
> >   #endif
>