snake bug
mlarkin discovered that if the treasure lands on the snake, you won't see it.
it's still there, but invisible. this fixes the display to draw the correct
character.
Index: snake.c
===
RCS file: /cvs/src/games/snake/snake.c,v
retrieving revision 1.29
diff -u -p -r1.29 snake.c
--- snake.c 24 Aug 2018 11:14:49 - 1.29
+++ snake.c 20 Jan 2019 03:47:03 -
@@ -844,8 +844,12 @@ pushsnake(void)
for (i = 4; i >= 0; i--)
if (same([i], [5]))
issame++;
- if (!issame)
- pchar([5], ' ');
+ if (!issame) {
+ char sp = ' ';
+ if (same(, [5]))
+ sp = TREASURE;
+ pchar([5], sp);
+ }
/* Need the following to catch you if you step on the snake's tail */
tmp.col = snake[5].col;
tmp.line = snake[5].line;
Re: [PATCH] systat reports inaccurate statistics for disk i/o speed
On 2019-01-19 16:09:05, Ted Unangst wrote:
> Bryan Linton wrote:
> > Hello tech@,
> >
> > I'd appreciate it if someone could review both this patch and my
> > analysis.
>
> There is no reason for etime to be a global. We can make it a local with the
> correct value in the two functions that use it, avoiding future problems as
> well.
>
I can confirm that your patch fixes the error as well, and is
probably the better option since it removes a global.
Thank you for reviewing everything.
--
Bryan
> Index: vmstat.c
> ===
> RCS file: /cvs/src/usr.bin/systat/vmstat.c,v
> retrieving revision 1.89
> diff -u -p -r1.89 vmstat.c
> --- vmstat.c 17 Nov 2018 23:10:08 - 1.89
> +++ vmstat.c 19 Jan 2019 21:06:38 -
> @@ -97,7 +97,6 @@ int select_vm(void);
> int vm_keyboard_callback(int);
>
> static time_t t;
> -static double etime;
> static float hertz;
> static int nintr;
> static long *intrloc;
> @@ -336,6 +335,7 @@ showkre(void)
> u_int64_t inttotal, intcnt;
> int i, l, c;
> static int failcnt = 0, first_run = 0;
> + double etime;
>
> if (state == TIME) {
> if (!first_run) {
> @@ -676,7 +676,9 @@ copyinfo(struct Info *from, struct Info
> static void
> dinfo(int dn, int c)
> {
> - double words, atime;
> + double words, atime, etime;
> +
> + etime = naptime;
>
> c += DISKCOL;
Re: [PATCH 6/7] Rework virtio_negotiate_features()
On Sat, Jan 19, 2019 at 05:37:34PM +0100, Stefan Fritsch wrote:
> Add a sc_driver_features field that is automatically used by
> virtio_negotiate_features() and during reinit.
>
> Make virtio_negotiate_features() return an error code. Virtio 1.0 has a
> special status bit for feature negotiation that means that negotiation
> can fail. Make virtio_negotiate_features() return an error code instead
> of the features.
>
> Make virtio_reinit_start() automatically call
> virtio_negotiate_features().
>
> Add a convenience function virtio_has_feature() to make checking bits
> easier.
>
> Add an error check in viomb for virtio_negotiate_features because it has
> some feature bits that may cause negotiation to fail. More error
> checking in the child drivers is still missing.
ok mlarkin
> ---
> sys/dev/fdt/virtio_mmio.c | 38 ++
> sys/dev/pci/virtio_pci.c | 39 +++
> sys/dev/pv/if_vio.c | 38 +-
> sys/dev/pv/vioblk.c | 25 +
> sys/dev/pv/viocon.c | 6 +++---
> sys/dev/pv/viomb.c| 9 +
> sys/dev/pv/viornd.c | 2 +-
> sys/dev/pv/vioscsi.c | 2 +-
> sys/dev/pv/virtio.c | 13 +++--
> sys/dev/pv/virtiovar.h| 15 ---
> sys/dev/pv/vmmci.c| 13 ++---
> 11 files changed, 110 insertions(+), 90 deletions(-)
>
> diff --git a/sys/dev/fdt/virtio_mmio.c b/sys/dev/fdt/virtio_mmio.c
> index 2d7a131125d..6e09fb0e76f 100644
> --- a/sys/dev/fdt/virtio_mmio.c
> +++ b/sys/dev/fdt/virtio_mmio.c
> @@ -91,8 +91,8 @@ void
> virtio_mmio_write_device_config_8(struct virtio_softc *, int, uint64_t);
> uint16_t virtio_mmio_read_queue_size(struct virtio_softc *, uint16_t);
> void virtio_mmio_setup_queue(struct virtio_softc *, struct virtqueue
> *, uint64_t);
> void virtio_mmio_set_status(struct virtio_softc *, int);
> -uint64_t virtio_mmio_negotiate_features(struct virtio_softc *, uint64_t,
> - const struct virtio_feature_name
> *);
> +int virtio_mmio_negotiate_features(struct virtio_softc *,
> +const struct virtio_feature_name *);
> int virtio_mmio_intr(void *);
>
> struct virtio_mmio_softc {
> @@ -300,28 +300,42 @@ virtio_mmio_detach(struct device *self, int flags)
> * Prints available / negotiated features if guest_feature_names != NULL and
> * VIRTIO_DEBUG is 1
> */
> -uint64_t
> -virtio_mmio_negotiate_features(struct virtio_softc *vsc, uint64_t
> guest_features,
> - const struct virtio_feature_name *guest_feature_names)
> +int
> +virtio_mmio_negotiate_features(struct virtio_softc *vsc,
> +const struct virtio_feature_name *guest_feature_names)
> {
> struct virtio_mmio_softc *sc = (struct virtio_mmio_softc *)vsc;
> uint64_t host, neg;
>
> + vsc->sc_active_features = 0;
> +
> /*
> - * indirect descriptors can be switched off by setting bit 1 in the
> - * driver flags, see config(8)
> + * We enable indirect descriptors by default. They can be switched
> + * off by setting bit 1 in the driver flags, see config(8).
>*/
> if (!(vsc->sc_dev.dv_cfdata->cf_flags & VIRTIO_CF_NO_INDIRECT) &&
> !(vsc->sc_child->dv_cfdata->cf_flags & VIRTIO_CF_NO_INDIRECT)) {
> - guest_features |= VIRTIO_F_RING_INDIRECT_DESC;
> - } else {
> + vsc->sc_driver_features |= VIRTIO_F_RING_INDIRECT_DESC;
> + } else if (guest_feature_names != NULL) {
> printf("RingIndirectDesc disabled by UKC\n");
> }
> + /*
> + * The driver must add VIRTIO_F_RING_EVENT_IDX if it supports it.
> + * If it did, check if it is disabled by bit 2 in the driver flags.
> + */
> + if ((vsc->sc_driver_features & VIRTIO_F_RING_EVENT_IDX) &&
> + ((vsc->sc_dev.dv_cfdata->cf_flags & VIRTIO_CF_NO_EVENT_IDX) ||
> + (vsc->sc_child->dv_cfdata->cf_flags & VIRTIO_CF_NO_EVENT_IDX))) {
> + if (guest_feature_names != NULL)
> + printf(" RingEventIdx disabled by UKC");
> + vsc->sc_driver_features &= ~(VIRTIO_F_RING_EVENT_IDX);
> + }
> +
> bus_space_write_4(sc->sc_iot, sc->sc_ioh,
> VIRTIO_MMIO_HOST_FEATURES_SEL, 0);
> host = bus_space_read_4(sc->sc_iot, sc->sc_ioh,
> VIRTIO_MMIO_HOST_FEATURES);
> - neg = host & guest_features;
> + neg = host & vsc->sc_driver_features;
> #if VIRTIO_DEBUG
> if (guest_feature_names)
> virtio_log_features(host, neg, guest_feature_names);
> @@ -330,13 +344,13 @@ virtio_mmio_negotiate_features(struct virtio_softc
> *vsc, uint64_t guest_features
> VIRTIO_MMIO_GUEST_FEATURES_SEL, 0);
> bus_space_write_4(sc->sc_iot, sc->sc_ioh,
> VIRTIO_MMIO_GUEST_FEATURES, neg);
> - vsc->sc_features = neg;
>
Re: [PATCH 7/7] Support virtio 1.0 for virtio_pci
On Sat, Jan 19, 2019 at 05:37:35PM +0100, Stefan Fritsch wrote:
> virtio 1.0 for virtio_mmio it not yet implemented, but 0.9 devices
> continue to work.
> ---
This one also should have someone who is well versed in PCI take a look.
-ml
> share/man/man4/virtio.4 | 11 +-
> sys/dev/pci/virtio_pci.c| 518 +++-
> sys/dev/pci/virtio_pcireg.h | 57
> sys/dev/pv/if_vio.c | 9 +-
> sys/dev/pv/virtiovar.h | 5 +
> 5 files changed, 531 insertions(+), 69 deletions(-)
>
> diff --git a/share/man/man4/virtio.4 b/share/man/man4/virtio.4
> index 5e60e2e0c32..dd9d4551715 100644
> --- a/share/man/man4/virtio.4
> +++ b/share/man/man4/virtio.4
> @@ -22,7 +22,7 @@
> .Nd VirtIO support driver
> .Sh SYNOPSIS
> .Cd "virtio* at fdt?"
> -.Cd "virtio* at pci?"
> +.Cd "virtio* at pci? flags 0x00"
> .Sh DESCRIPTION
> The
> .Nm
> @@ -56,7 +56,14 @@ control interface
> The
> .Nm
> driver conforms to the virtio 0.9.5 specification.
> -The virtio 1.0 standard is not supported, yet.
> +The virtio 1.0 standard is only supported for pci devices.
> +.Pp
> +By default 0.9 is preferred over 1.0.
> +This can be changed by setting the bit 0x4 in the flags.
> +.Pp
> +Setting the bit 0x8 in the flags disables the 1.0 support completely.
> +.Sh BUGS
> +.Nm Big-endian architectures are not yet supported.
> .Sh SEE ALSO
> .Xr intro 4
> .Sh HISTORY
> diff --git a/sys/dev/pci/virtio_pci.c b/sys/dev/pci/virtio_pci.c
> index f370f513e85..591da61290a 100644
> --- a/sys/dev/pci/virtio_pci.c
> +++ b/sys/dev/pci/virtio_pci.c
> @@ -35,11 +35,16 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> #include
>
> +#define DNPRINTF(n,x...) \
> +do { if (VIRTIO_DEBUG >= n) printf(x); } while(0)
> +
> +
> /*
> * XXX: Before being used on big endian arches, the access to config
> registers
> * XXX: needs to be reviewed/fixed. The non-device specific registers are
> @@ -52,6 +57,8 @@ struct virtio_pci_softc;
>
> int virtio_pci_match(struct device *, void *, void *);
> void virtio_pci_attach(struct device *, struct device *, void *);
> +int virtio_pci_attach_09(struct virtio_pci_softc *sc, struct
> pci_attach_args *pa);
> +int virtio_pci_attach_10(struct virtio_pci_softc *sc, struct
> pci_attach_args *pa);
> int virtio_pci_detach(struct device *, int);
>
> void virtio_pci_kick(struct virtio_softc *, uint16_t);
> @@ -67,8 +74,8 @@ voidvirtio_pci_write_device_config_8(struct
> virtio_softc *, int, uint64_t);
> uint16_t virtio_pci_read_queue_size(struct virtio_softc *, uint16_t);
> void virtio_pci_setup_queue(struct virtio_softc *, struct virtqueue
> *, uint64_t);
> void virtio_pci_set_status(struct virtio_softc *, int);
> -int virtio_pci_negotiate_features(struct virtio_softc *,
> -const struct virtio_feature_name *);
> +int virtio_pci_negotiate_features(struct virtio_softc *, const
> struct virtio_feature_name *);
> +int virtio_pci_negotiate_features_10(struct virtio_softc *, const
> struct virtio_feature_name *);
> void virtio_pci_set_msix_queue_vector(struct virtio_pci_softc *,
> uint32_t, uint16_t);
> void virtio_pci_set_msix_config_vector(struct virtio_pci_softc *,
> uint16_t);
> int virtio_pci_msix_establish(struct virtio_pci_softc *, struct
> pci_attach_args *, int, int (*)(void *), void *);
> @@ -80,6 +87,10 @@ intvirtio_pci_legacy_intr_mpsafe(void *);
> int virtio_pci_config_intr(void *);
> int virtio_pci_queue_intr(void *);
> int virtio_pci_shared_queue_intr(void *);
> +int virtio_pci_find_cap(struct virtio_pci_softc *sc, int cfg_type,
> void *buf, int buflen);
> +#if VIRTIO_DEBUG
> +void virtio_pci_dump_caps(struct virtio_pci_softc *sc);
> +#endif
>
> enum irq_type {
> IRQ_NO_MSIX,
> @@ -96,9 +107,14 @@ struct virtio_pci_softc {
> bus_space_handle_t sc_ioh;
> bus_size_t sc_iosize;
>
> + bus_space_tag_t sc_bars_iot[4];
> + bus_space_handle_t sc_bars_ioh[4];
> + bus_size_t sc_bars_iosize[4];
> +
> bus_space_tag_t sc_notify_iot;
> bus_space_handle_t sc_notify_ioh;
> bus_size_t sc_notify_iosize;
> + unsigned intsc_notify_off_multiplier;
>
> bus_space_tag_t sc_devcfg_iot;
> bus_space_handle_t sc_devcfg_ioh;
> @@ -145,14 +161,69 @@ struct virtio_ops virtio_pci_ops = {
> virtio_pci_poll_intr,
> };
>
> +static inline
> +uint64_t _cread(struct virtio_pci_softc *sc, unsigned off, unsigned size)
> +{
> + uint64_t val;
> + switch (size) {
> + case 1:
> + val = bus_space_read_1(sc->sc_iot, sc->sc_ioh, off);
> + break;
> + case 2:
> + val = bus_space_read_2(sc->sc_iot,
Re: [PATCH 4/7] virtio_pci: Split bus space handles
On Sat, Jan 19, 2019 at 05:37:32PM +0100, Stefan Fritsch wrote:
> In virtio_pci 1.0, different parts of the register set may be located in
> different BARs. Use subregions to make the access independent of the
> virtio version.
> ---
This one will need someone more well versed in PCI BARs/etc to check.
-ml
> sys/dev/pci/virtio_pci.c | 114 +++
> 1 file changed, 80 insertions(+), 34 deletions(-)
>
> diff --git a/sys/dev/pci/virtio_pci.c b/sys/dev/pci/virtio_pci.c
> index 97e5607a55e..d69db1968d0 100644
> --- a/sys/dev/pci/virtio_pci.c
> +++ b/sys/dev/pci/virtio_pci.c
> @@ -55,6 +55,7 @@ voidvirtio_pci_attach(struct device *,
> struct device *, void *);
> int virtio_pci_detach(struct device *, int);
>
> void virtio_pci_kick(struct virtio_softc *, uint16_t);
> +int virtio_pci_adjust_config_region(struct virtio_pci_softc *);
> uint8_t virtio_pci_read_device_config_1(struct virtio_softc *,
> int);
> uint16_t virtio_pci_read_device_config_2(struct virtio_softc *, int);
> uint32_t virtio_pci_read_device_config_4(struct virtio_softc *, int);
> @@ -87,14 +88,33 @@ enum irq_type {
> struct virtio_pci_softc {
> struct virtio_softc sc_sc;
> pci_chipset_tag_t sc_pc;
> + pcitag_tsc_ptag;
>
> bus_space_tag_t sc_iot;
> bus_space_handle_t sc_ioh;
> bus_size_t sc_iosize;
>
> + bus_space_tag_t sc_notify_iot;
> + bus_space_handle_t sc_notify_ioh;
> + bus_size_t sc_notify_iosize;
> +
> + bus_space_tag_t sc_devcfg_iot;
> + bus_space_handle_t sc_devcfg_ioh;
> + bus_size_t sc_devcfg_iosize;
> + /*
> + * With 0.9, the offset of the devcfg region in the io bar changes
> + * depending on MSI-X being enabled or not.
> + * With 1.0, this field is still used to remember if MSI-X is enabled
> + * or not.
> + */
> + unsigned intsc_devcfg_offset;
> +
> + bus_space_tag_t sc_isr_iot;
> + bus_space_handle_t sc_isr_ioh;
> + bus_size_t sc_isr_iosize;
> +
> void*sc_ih[MAX_MSIX_VECS];
>
> - int sc_config_offset;
> enum irq_type sc_irq_type;
> };
>
> @@ -213,9 +233,8 @@ virtio_pci_attach(struct device *parent, struct device
> *self, void *aux)
>
> vsc->sc_ops = _pci_ops;
> sc->sc_pc = pc;
> + sc->sc_ptag = pa->pa_tag;
> vsc->sc_dmat = pa->pa_dmat;
> - sc->sc_config_offset = VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI;
> - sc->sc_irq_type = IRQ_NO_MSIX;
>
> /*
>* For virtio, ignore normal MSI black/white-listing depending on the
> @@ -229,11 +248,33 @@ virtio_pci_attach(struct device *parent, struct device
> *self, void *aux)
> return;
> }
>
> + if (bus_space_subregion(sc->sc_iot, sc->sc_ioh,
> + VIRTIO_CONFIG_QUEUE_NOTIFY, 2, >sc_notify_ioh) != 0) {
> + printf("%s: can't map notify i/o space\n",
> + vsc->sc_dev.dv_xname);
> + return;
> + }
> + sc->sc_notify_iosize = 2;
> + sc->sc_notify_iot = sc->sc_iot;
> +
> + if (bus_space_subregion(sc->sc_iot, sc->sc_ioh,
> + VIRTIO_CONFIG_ISR_STATUS, 1, >sc_isr_ioh) != 0) {
> + printf("%s: can't map isr i/o space\n",
> + vsc->sc_dev.dv_xname);
> + return;
> + }
> + sc->sc_isr_iosize = 1;
> + sc->sc_isr_iot = sc->sc_iot;
> +
> + sc->sc_devcfg_offset = VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI;
> + sc->sc_irq_type = IRQ_NO_MSIX;
> + if (virtio_pci_adjust_config_region(sc) != 0)
> + return;
> +
> virtio_device_reset(vsc);
> virtio_pci_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_ACK);
> virtio_pci_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER);
>
> - /* XXX: use softc as aux... */
> vsc->sc_childdevid = id;
> vsc->sc_child = NULL;
> config_found(self, sc, NULL);
> @@ -312,6 +353,20 @@ virtio_pci_detach(struct device *self, int flags)
> return 0;
> }
>
> +int
> +virtio_pci_adjust_config_region(struct virtio_pci_softc *sc)
> +{
> + sc->sc_devcfg_iosize = sc->sc_iosize - sc->sc_devcfg_offset;
> + sc->sc_devcfg_iot = sc->sc_iot;
> + if (bus_space_subregion(sc->sc_iot, sc->sc_ioh, sc->sc_devcfg_offset,
> + sc->sc_devcfg_iosize, >sc_devcfg_ioh) != 0) {
> + printf("%s: can't map config i/o space\n",
> + sc->sc_sc.sc_dev.dv_xname);
> + return 1;
> + }
> + return 0;
> +}
> +
> /*
> * Feature negotiation.
> * Prints available / negotiated features if guest_feature_names != NULL and
> @@ -359,24 +414,21 @@ uint8_t
> virtio_pci_read_device_config_1(struct virtio_softc *vsc, int index)
> {
> struct virtio_pci_softc *sc = (struct virtio_pci_softc *)vsc;
> -
Re: [PATCH 5/7] virtio_pci: Move msix vector config into functions
On Sat, Jan 19, 2019 at 05:37:33PM +0100, Stefan Fritsch wrote:
> ---
> sys/dev/pci/virtio_pci.c | 38 --
> 1 file changed, 24 insertions(+), 14 deletions(-)
>
ok mlarkin
> diff --git a/sys/dev/pci/virtio_pci.c b/sys/dev/pci/virtio_pci.c
> index d69db1968d0..7be93684a68 100644
> --- a/sys/dev/pci/virtio_pci.c
> +++ b/sys/dev/pci/virtio_pci.c
> @@ -69,6 +69,8 @@ voidvirtio_pci_setup_queue(struct
> virtio_softc *, struct virtqueue *, uint64_t
> void virtio_pci_set_status(struct virtio_softc *, int);
> uint64_t virtio_pci_negotiate_features(struct virtio_softc *, uint64_t,
> const struct virtio_feature_name
> *);
> +void virtio_pci_set_msix_queue_vector(struct virtio_pci_softc *,
> uint32_t, uint16_t);
> +void virtio_pci_set_msix_config_vector(struct virtio_pci_softc *,
> uint16_t);
> int virtio_pci_msix_establish(struct virtio_pci_softc *, struct
> pci_attach_args *, int, int (*)(void *), void *);
> int virtio_pci_setup_msix(struct virtio_pci_softc *, struct
> pci_attach_args *, int);
> void virtio_pci_free_irqs(struct virtio_pci_softc *);
> @@ -503,6 +505,22 @@ virtio_pci_msix_establish(struct virtio_pci_softc *sc,
> return 0;
> }
>
> +void
> +virtio_pci_set_msix_queue_vector(struct virtio_pci_softc *sc, uint32_t idx,
> uint16_t vector)
> +{
> + bus_space_write_2(sc->sc_iot, sc->sc_ioh,
> + VIRTIO_CONFIG_QUEUE_SELECT, idx);
> + bus_space_write_2(sc->sc_iot, sc->sc_ioh,
> + VIRTIO_MSI_QUEUE_VECTOR, vector);
> +}
> +
> +void
> +virtio_pci_set_msix_config_vector(struct virtio_pci_softc *sc, uint16_t
> vector)
> +{
> + bus_space_write_2(sc->sc_iot, sc->sc_ioh,
> + VIRTIO_MSI_CONFIG_VECTOR, vector);
> +}
> +
> void
> virtio_pci_free_irqs(struct virtio_pci_softc *sc)
> {
> @@ -511,10 +529,8 @@ virtio_pci_free_irqs(struct virtio_pci_softc *sc)
>
> if (sc->sc_devcfg_offset == VIRTIO_CONFIG_DEVICE_CONFIG_MSI) {
> for (i = 0; i < vsc->sc_nvqs; i++) {
> - bus_space_write_2(sc->sc_iot, sc->sc_ioh,
> - VIRTIO_CONFIG_QUEUE_SELECT, i);
> - bus_space_write_2(sc->sc_iot, sc->sc_ioh,
> - VIRTIO_MSI_QUEUE_VECTOR, VIRTIO_MSI_NO_VECTOR);
> + virtio_pci_set_msix_queue_vector(sc, i,
> + VIRTIO_MSI_NO_VECTOR);
> }
> }
>
> @@ -540,6 +556,7 @@ virtio_pci_setup_msix(struct virtio_pci_softc *sc, struct
> pci_attach_args *pa,
> return 1;
> sc->sc_devcfg_offset = VIRTIO_CONFIG_DEVICE_CONFIG_MSI;
> virtio_pci_adjust_config_region(sc);
> + virtio_pci_set_msix_config_vector(sc, 0);
>
> if (shared) {
> if (virtio_pci_msix_establish(sc, pa, 1,
> @@ -547,22 +564,15 @@ virtio_pci_setup_msix(struct virtio_pci_softc *sc,
> struct pci_attach_args *pa,
> goto fail;
> }
>
> - for (i = 0; i < vsc->sc_nvqs; i++) {
> - bus_space_write_2(sc->sc_iot, sc->sc_ioh,
> - VIRTIO_CONFIG_QUEUE_SELECT, i);
> - bus_space_write_2(sc->sc_iot, sc->sc_ioh,
> - VIRTIO_MSI_QUEUE_VECTOR, 1);
> - }
> + for (i = 0; i < vsc->sc_nvqs; i++)
> + virtio_pci_set_msix_queue_vector(sc, i, 1);
> } else {
> for (i = 0; i <= vsc->sc_nvqs; i++) {
> if (virtio_pci_msix_establish(sc, pa, i + 1,
> virtio_pci_queue_intr, >sc_vqs[i])) {
> goto fail;
> }
> - bus_space_write_2(sc->sc_iot, sc->sc_ioh,
> - VIRTIO_CONFIG_QUEUE_SELECT, i);
> - bus_space_write_2(sc->sc_iot, sc->sc_ioh,
> - VIRTIO_MSI_QUEUE_VECTOR, i + 1);
> + virtio_pci_set_msix_queue_vector(sc, i, i + 1);
> }
> }
>
> --
> 2.19.0
>
Re: [PATCH 3/7] virtio: Add a few feature bit defines and names
On Sat, Jan 19, 2019 at 05:37:31PM +0100, Stefan Fritsch wrote:
> ---
> sys/dev/pv/if_vio.c| 84 +++---
> sys/dev/pv/vioblk.c| 3 ++
> sys/dev/pv/vioblkreg.h | 21 ++-
> sys/dev/pv/virtio.c| 1 +
> 4 files changed, 62 insertions(+), 47 deletions(-)
>
ok mlarkin
> diff --git a/sys/dev/pv/if_vio.c b/sys/dev/pv/if_vio.c
> index a4cd80af62d..bfc7cfd1ddc 100644
> --- a/sys/dev/pv/if_vio.c
> +++ b/sys/dev/pv/if_vio.c
> @@ -68,25 +68,29 @@
> #define VIRTIO_NET_CONFIG_STATUS 6 /* 16bit */
>
> /* Feature bits */
> -#define VIRTIO_NET_F_CSUM(1ULL<<0)
> -#define VIRTIO_NET_F_GUEST_CSUM (1ULL<<1)
> -#define VIRTIO_NET_F_MAC (1ULL<<5)
> -#define VIRTIO_NET_F_GSO (1ULL<<6)
> -#define VIRTIO_NET_F_GUEST_TSO4 (1ULL<<7)
> -#define VIRTIO_NET_F_GUEST_TSO6 (1ULL<<8)
> -#define VIRTIO_NET_F_GUEST_ECN (1ULL<<9)
> -#define VIRTIO_NET_F_GUEST_UFO (1ULL<<10)
> -#define VIRTIO_NET_F_HOST_TSO4 (1ULL<<11)
> -#define VIRTIO_NET_F_HOST_TSO6 (1ULL<<12)
> -#define VIRTIO_NET_F_HOST_ECN(1ULL<<13)
> -#define VIRTIO_NET_F_HOST_UFO(1ULL<<14)
> -#define VIRTIO_NET_F_MRG_RXBUF (1ULL<<15)
> -#define VIRTIO_NET_F_STATUS (1ULL<<16)
> -#define VIRTIO_NET_F_CTRL_VQ (1ULL<<17)
> -#define VIRTIO_NET_F_CTRL_RX (1ULL<<18)
> -#define VIRTIO_NET_F_CTRL_VLAN (1ULL<<19)
> -#define VIRTIO_NET_F_CTRL_RX_EXTRA (1ULL<<20)
> -#define VIRTIO_NET_F_GUEST_ANNOUNCE (1ULL<<21)
> +#define VIRTIO_NET_F_CSUM(1ULL<<0)
> +#define VIRTIO_NET_F_GUEST_CSUM (1ULL<<1)
> +#define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS(1ULL<<2)
> +#define VIRTIO_NET_F_MTU(1ULL<<3)
> +#define VIRTIO_NET_F_MAC (1ULL<<5)
> +#define VIRTIO_NET_F_GSO (1ULL<<6)
> +#define VIRTIO_NET_F_GUEST_TSO4 (1ULL<<7)
> +#define VIRTIO_NET_F_GUEST_TSO6 (1ULL<<8)
> +#define VIRTIO_NET_F_GUEST_ECN (1ULL<<9)
> +#define VIRTIO_NET_F_GUEST_UFO (1ULL<<10)
> +#define VIRTIO_NET_F_HOST_TSO4 (1ULL<<11)
> +#define VIRTIO_NET_F_HOST_TSO6 (1ULL<<12)
> +#define VIRTIO_NET_F_HOST_ECN(1ULL<<13)
> +#define VIRTIO_NET_F_HOST_UFO(1ULL<<14)
> +#define VIRTIO_NET_F_MRG_RXBUF (1ULL<<15)
> +#define VIRTIO_NET_F_STATUS (1ULL<<16)
> +#define VIRTIO_NET_F_CTRL_VQ (1ULL<<17)
> +#define VIRTIO_NET_F_CTRL_RX (1ULL<<18)
> +#define VIRTIO_NET_F_CTRL_VLAN (1ULL<<19)
> +#define VIRTIO_NET_F_CTRL_RX_EXTRA (1ULL<<20)
> +#define VIRTIO_NET_F_GUEST_ANNOUNCE (1ULL<<21)
> +#define VIRTIO_NET_F_MQ (1ULL<<22)
> +#define VIRTIO_NET_F_CTRL_MAC_ADDR (1ULL<<23)
>
> /*
> * Config(8) flags. The lowest byte is reserved for generic virtio stuff.
> @@ -97,25 +101,29 @@
>
> static const struct virtio_feature_name virtio_net_feature_names[] = {
> #if VIRTIO_DEBUG
> - { VIRTIO_NET_F_CSUM,"CSum" },
> - { VIRTIO_NET_F_GUEST_CSUM, "GuestCSum" },
> - { VIRTIO_NET_F_MAC, "MAC" },
> - { VIRTIO_NET_F_GSO, "GSO" },
> - { VIRTIO_NET_F_GUEST_TSO4, "GuestTSO4" },
> - { VIRTIO_NET_F_GUEST_TSO6, "GuestTSO6" },
> - { VIRTIO_NET_F_GUEST_ECN, "GuestECN" },
> - { VIRTIO_NET_F_GUEST_UFO, "GuestUFO" },
> - { VIRTIO_NET_F_HOST_TSO4, "HostTSO4" },
> - { VIRTIO_NET_F_HOST_TSO6, "HostTSO6" },
> - { VIRTIO_NET_F_HOST_ECN,"HostECN" },
> - { VIRTIO_NET_F_HOST_UFO,"HostUFO" },
> - { VIRTIO_NET_F_MRG_RXBUF, "MrgRXBuf" },
> - { VIRTIO_NET_F_STATUS, "Status" },
> - { VIRTIO_NET_F_CTRL_VQ, "CtrlVQ" },
> - { VIRTIO_NET_F_CTRL_RX, "CtrlRX" },
> - { VIRTIO_NET_F_CTRL_VLAN, "CtrlVLAN" },
> - { VIRTIO_NET_F_CTRL_RX_EXTRA, "CtrlRXExtra" },
> - { VIRTIO_NET_F_GUEST_ANNOUNCE, "GuestAnnounce" },
> + { VIRTIO_NET_F_CSUM,"CSum" },
> + { VIRTIO_NET_F_GUEST_CSUM, "GuestCSum" },
> + { VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, "CtrlGuestOffl" },
> + { VIRTIO_NET_F_MTU, "MTU", },
> + { VIRTIO_NET_F_MAC, "MAC" },
> + { VIRTIO_NET_F_GSO, "GSO" },
> + { VIRTIO_NET_F_GUEST_TSO4, "GuestTSO4" },
> + { VIRTIO_NET_F_GUEST_TSO6, "GuestTSO6" },
> + { VIRTIO_NET_F_GUEST_ECN, "GuestECN" },
> + { VIRTIO_NET_F_GUEST_UFO, "GuestUFO" },
> + { VIRTIO_NET_F_HOST_TSO4, "HostTSO4" },
> + {
Re: kcov: trace cmp
On Fri, Jan 18, 2019 at 07:34:35PM +0100, Anton Lindqvist wrote:
> You're right, updated diff.
OK bluhm@
> Index: share/man/man4/kcov.4
> ===
> RCS file: /cvs/src/share/man/man4/kcov.4,v
> retrieving revision 1.6
> diff -u -p -r1.6 kcov.4
> --- share/man/man4/kcov.4 27 Dec 2018 19:33:08 - 1.6
> +++ share/man/man4/kcov.4 18 Jan 2019 18:26:17 -
> @@ -62,9 +62,35 @@ Enable code coverage tracing for the cur
> The
> .Fa mode
> must be one of the following:
> -.Bl -tag -width KCOV_MODE_TRACE_PC
> +.Bl -ohang
> .It Dv KCOV_MODE_TRACE_PC
> Trace the kernel program counter.
> +.It Dv KCOV_MODE_TRACE_CMP
> +Trace comparison instructions and switch statements.
> +For switch statements, the number of traced comparison instructions is equal
> to
> +the number of switch cases.
> +Each traced comparison instruction is represented by 4 entries in the
> coverage
> +buffer:
> +.Bl -enum
> +.It
> +A mask where the least significant bit is set if one of the comparison
> operands
> +is a compile-time constant, which is always true for switch statements.
> +The remaining bits represents the log2 size of the operands, ranging from 0
> to
> +3.
> +.It
> +First comparison operand.
> +For switch statements, this operand corresponds to the case value.
> +.It
> +Second comparison operand.
> +For switch statements, this operand corresponds to the value passed to
> switch.
> +.It
> +Kernel program counter where the comparison instruction took place.
> +.El
> +.Pp
> +In this mode, the first entry in the coverage buffer reflects the number of
> +traced comparison instructions.
> +Thus, the effective number of entries in the coverage buffer is given by
> +multiplying the first entry by 4.
> .El
> .It Dv KIODISABLE Fa void
> Disable code coverage tracing for the current thread.
> Index: sys/arch/amd64/conf/Makefile.amd64
> ===
> RCS file: /cvs/src/sys/arch/amd64/conf/Makefile.amd64,v
> retrieving revision 1.108
> diff -u -p -r1.108 Makefile.amd64
> --- sys/arch/amd64/conf/Makefile.amd647 Jan 2019 20:24:59 -
> 1.108
> +++ sys/arch/amd64/conf/Makefile.amd6418 Jan 2019 18:26:17 -
> @@ -95,7 +95,7 @@ LINKFLAGS+= -S
> .endif
>
> .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
> -PROF=-fsanitize-coverage=trace-pc
> +PROF=-fsanitize-coverage=trace-pc,trace-cmp
> .endif
>
> %LOAD
> @@ -152,7 +152,7 @@ vers.o: ${SYSTEM_DEP:Ngap.o}
>
> .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
> kcov.o: $S/dev/kcov.c
> - ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc,trace-cmp
> .endif
>
> clean:
> Index: sys/arch/i386/conf/Makefile.i386
> ===
> RCS file: /cvs/src/sys/arch/i386/conf/Makefile.i386,v
> retrieving revision 1.130
> diff -u -p -r1.130 Makefile.i386
> --- sys/arch/i386/conf/Makefile.i386 30 Oct 2018 11:08:30 - 1.130
> +++ sys/arch/i386/conf/Makefile.i386 18 Jan 2019 18:26:17 -
> @@ -97,7 +97,7 @@ LINKFLAGS+= -S
> .endif
>
> .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
> -PROF=-fsanitize-coverage=trace-pc
> +PROF=-fsanitize-coverage=trace-pc,trace-cmp
> .endif
>
> %LOAD
> @@ -148,7 +148,7 @@ vers.o: ${SYSTEM_DEP:Ngap.o}
>
> .if ${SYSTEM_OBJ:Mkcov.o} && ${COMPILER_VERSION:Mclang}
> kcov.o: $S/dev/kcov.c
> - ${NORMAL_C} -fno-sanitize-coverage=trace-pc
> + ${NORMAL_C} -fno-sanitize-coverage=trace-pc,trace-cmp
> .endif
>
> clean:
> Index: sys/dev/kcov.c
> ===
> RCS file: /cvs/src/sys/dev/kcov.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 kcov.c
> --- sys/dev/kcov.c16 Jan 2019 19:27:07 - 1.10
> +++ sys/dev/kcov.c18 Jan 2019 18:26:17 -
> @@ -26,6 +26,9 @@
>
> #include
>
> +#define KCOV_CMP_CONST 0x1
> +#define KCOV_CMP_SIZE(x) ((x) << 1)
> +
> /* #define KCOV_DEBUG */
> #ifdef KCOV_DEBUG
> #define DPRINTF(x...) do { if (kcov_debug) printf(x); } while (0)
> @@ -99,12 +102,140 @@ __sanitizer_cov_trace_pc(void)
> return;
>
> idx = kd->kd_buf[0];
> - if (idx < kd->kd_nmemb) {
> + if (idx + 1 <= kd->kd_nmemb) {
> kd->kd_buf[idx + 1] = (uintptr_t)__builtin_return_address(0);
> kd->kd_buf[0] = idx + 1;
> }
> }
>
> +/*
> + * Compiling the kernel with the `-fsanitize-coverage=trace-cmp' option will
> + * cause the following function to be called upon integer comparisons and
> switch
> + * statements.
> + *
> + * If kcov is enabled for the current thread, the comparison will be stored
> in
> + * its corresponding coverage buffer.
> + */
> +void
> +trace_cmp(uint64_t type, uint64_t arg1, uint64_t arg2, uintptr_t pc)
> +{
> +
Re: bwfm(4): show media mode, TX rate, and RSSI
On Tue, Jan 15, 2019 at 10:01:42PM +0100, Stefan Sperling wrote:
> This diff makes 'ifconfig bwfm0' display whether 11n is active or not,
> what the current Tx rate is, and show up-to-date RSSI measurements.
> I'm leaving display of 11ac Tx rates for future work because that will
> require a change in struct ieet80211_node.
>
> Tested with 'bwfm0 at pci1 dev 0 function 0 "Broadcom BCM4350" rev 0x08: msi'
> as a client with 11a and 11n APs, and in hostap mode with an iwn(4) client.
>
> I had a bwfm(4) USB device as well somewhere, but can't find it :-/
>
> Index: bwfm.c
> ===
> RCS file: /cvs/src/sys/dev/ic/bwfm.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 bwfm.c
> --- bwfm.c25 Jul 2018 20:37:11 - 1.54
> +++ bwfm.c15 Jan 2019 20:39:22 -
> @@ -59,6 +59,8 @@ void bwfm_start(struct ifnet *);
> void bwfm_init(struct ifnet *);
> void bwfm_stop(struct ifnet *);
> void bwfm_watchdog(struct ifnet *);
> +void bwfm_update_node(void *, struct ieee80211_node *);
> +void bwfm_update_nodes(struct bwfm_softc *);
> int bwfm_ioctl(struct ifnet *, u_long, caddr_t);
> int bwfm_media_change(struct ifnet *);
>
> @@ -549,10 +551,136 @@ bwfm_watchdog(struct ifnet *ifp)
> ieee80211_watchdog(ifp);
> }
>
> +extern struct ifmedia_baudrate ifmedia_baudrate_descriptions[];
> +
> +/*
> + * This function might lie because some MCS have equivalent baudrates.
> + * However, this is the best we can do given that the firmware only
> + * reports Tx baudrates.
> + */
> +uint64_t
> +bwfm_rate2mword(uint64_t baudrate)
> +{
> + int i;
> + uint64_t mword;
> +
> + for (i = 0; ifmedia_baudrate_descriptions[i].ifmb_word != 0; i++) {
> + mword = ifmedia_baudrate_descriptions[i].ifmb_word;
> + if (!IFM_TYPE_MATCH(IFM_IEEE80211, mword))
> + continue;
> + if (ifmedia_baudrate_descriptions[i].ifmb_baudrate == baudrate)
> + return mword;
> + }
> +
> + /* Not known. */
> + return 0;
> +}
> +
> +void
> +bwfm_update_node(void *arg, struct ieee80211_node *ni)
> +{
> + struct bwfm_softc *sc = arg;
> + struct ieee80211com *ic = >sc_ic;
> + struct bwfm_sta_info sta;
> + uint32_t flags;
> + int8_t rssi;
> + uint32_t txrate;
> + uint64_t mword;
> + int i;
> +
> + memset(, 0, sizeof(sta));
> + memcpy((uint8_t *), ni->ni_macaddr, sizeof(ni->ni_macaddr));
> +
> + if (bwfm_fwvar_var_get_data(sc, "sta_info", , sizeof(sta)))
> + return;
> +
> + if (!IEEE80211_ADDR_EQ(ni->ni_macaddr, sta.ea))
> + return;
> +
> + if (le16toh(sta.ver) < 4)
> + return;
> +
> + flags = le32toh(sta.flags);
> + if ((flags & BWFM_STA_SCBSTATS) == 0)
> + return;
> +
> + rssi = 0;
> + for (i = 0; i < BWFM_ANT_MAX; i++) {
> + if (sta.rssi[i] >= 0)
> + continue;
> + if (rssi == 0 || sta.rssi[i] > rssi)
> + rssi = sta.rssi[i];
> + }
> + if (rssi)
> + ni->ni_rssi = rssi;
> +
> + txrate = le32toh(sta.tx_rate);
> + if (txrate == 0x) /* Seen this happening during association. */
> + return;
> +
> + mword = bwfm_rate2mword(txrate * 1000);
> + if ((sta.flags & BWFM_STA_N_CAP) &&
> + IFM_SUBTYPE(mword) >= IFM_IEEE80211_HT_MCS0) {
> + /* Tell net80211 that firmware has negotiated 11n. */
> + ni->ni_flags |= IEEE80211_NODE_HT;
> + if (ic->ic_curmode < IEEE80211_MODE_11N)
> + ieee80211_setmode(ic, IEEE80211_MODE_11N);
> +
> + if ((sta.flags & BWFM_STA_VHT_CAP) &&
> + IFM_SUBTYPE(mword) >= IFM_IEEE80211_VHT_MCS0) {
> + /* TODO: Can't store VHT MCS in ni yet... */
> + } else
> + ni->ni_txmcs = ieee80211_media2mcs(mword);
> + } else {
> + /*
> + * In 11n mode a fallback to legacy rates is transparent
> + * to net80211. Just pretend we were using MCS 0.
> + */
> + if (ni->ni_flags & IEEE80211_NODE_HT)
> + ni->ni_txmcs = 0;
> + else {
> + /* We're in 11a/g mode. Map to a legacy rate. */
> + for (i = 0; i < ni->ni_rates.rs_nrates; i++) {
> + uint8_t rate = ni->ni_rates.rs_rates[i];
> + rate &= IEEE80211_RATE_VAL;
> + if (rate / 2 >= txrate / 1000) {
> + ni->ni_txrate = i;
> + break;
> + }
> + }
> + }
> + }
> +}
> +
> +void
> +bwfm_update_nodes(struct bwfm_softc *sc)
> +{
> + struct ieee80211com *ic = >sc_ic;
> + struct ieee80211_node *ni;
> +
> + switch
Re: [PATCH] systat reports inaccurate statistics for disk i/o speed
Bryan Linton wrote:
> Hello tech@,
>
> I'd appreciate it if someone could review both this patch and my
> analysis.
There is no reason for etime to be a global. We can make it a local with the
correct value in the two functions that use it, avoiding future problems as
well.
Index: vmstat.c
===
RCS file: /cvs/src/usr.bin/systat/vmstat.c,v
retrieving revision 1.89
diff -u -p -r1.89 vmstat.c
--- vmstat.c17 Nov 2018 23:10:08 - 1.89
+++ vmstat.c19 Jan 2019 21:06:38 -
@@ -97,7 +97,6 @@ int select_vm(void);
int vm_keyboard_callback(int);
static time_t t;
-static double etime;
static float hertz;
static int nintr;
static long *intrloc;
@@ -336,6 +335,7 @@ showkre(void)
u_int64_t inttotal, intcnt;
int i, l, c;
static int failcnt = 0, first_run = 0;
+ double etime;
if (state == TIME) {
if (!first_run) {
@@ -676,7 +676,9 @@ copyinfo(struct Info *from, struct Info
static void
dinfo(int dn, int c)
{
- double words, atime;
+ double words, atime, etime;
+
+ etime = naptime;
c += DISKCOL;
Re: [PATCH 1/7] virtio: adjust virtio_setup_queue prototype for 1.0
On Sat, Jan 19, 2019 at 05:37:29PM +0100, Stefan Fritsch wrote:
> Make it take an address instead of a PFN.
> Pass the virtqueue pointer. In virtio 1.0, more information has to be
> configured in the device. Also call virtio_setup_queue() after the
> information has been filled in.
> ---
> sys/dev/fdt/virtio_mmio.c | 11 +++
> sys/dev/pci/virtio_pci.c | 11 ++-
> sys/dev/pv/virtio.c | 11 ---
> sys/dev/pv/virtiovar.h| 2 +-
> 4 files changed, 18 insertions(+), 17 deletions(-)
>
This part reads ok to me, thanks for splitting the diff up.
> diff --git a/sys/dev/fdt/virtio_mmio.c b/sys/dev/fdt/virtio_mmio.c
> index 11867a43173..3a74e647c19 100644
> --- a/sys/dev/fdt/virtio_mmio.c
> +++ b/sys/dev/fdt/virtio_mmio.c
> @@ -89,7 +89,7 @@ void
> virtio_mmio_write_device_config_2(struct virtio_softc *, int, uint16_t);
> void virtio_mmio_write_device_config_4(struct virtio_softc *, int,
> uint32_t);
> void virtio_mmio_write_device_config_8(struct virtio_softc *, int,
> uint64_t);
> uint16_t virtio_mmio_read_queue_size(struct virtio_softc *, uint16_t);
> -void virtio_mmio_setup_queue(struct virtio_softc *, uint16_t,
> uint32_t);
> +void virtio_mmio_setup_queue(struct virtio_softc *, struct virtqueue
> *, uint64_t);
> void virtio_mmio_set_status(struct virtio_softc *, int);
> uint32_t virtio_mmio_negotiate_features(struct virtio_softc *, uint32_t,
> const struct virtio_feature_name
> *);
> @@ -151,15 +151,18 @@ virtio_mmio_read_queue_size(struct virtio_softc *vsc,
> uint16_t idx)
> }
>
> void
> -virtio_mmio_setup_queue(struct virtio_softc *vsc, uint16_t idx, uint32_t
> addr)
> +virtio_mmio_setup_queue(struct virtio_softc *vsc, struct virtqueue *vq,
> +uint64_t addr)
> {
> struct virtio_mmio_softc *sc = (struct virtio_mmio_softc *)vsc;
> - bus_space_write_4(sc->sc_iot, sc->sc_ioh, VIRTIO_MMIO_QUEUE_SEL, idx);
> + bus_space_write_4(sc->sc_iot, sc->sc_ioh, VIRTIO_MMIO_QUEUE_SEL,
> + vq->vq_index);
> bus_space_write_4(sc->sc_iot, sc->sc_ioh, VIRTIO_MMIO_QUEUE_NUM,
> bus_space_read_4(sc->sc_iot, sc->sc_ioh,
> VIRTIO_MMIO_QUEUE_NUM_MAX));
> bus_space_write_4(sc->sc_iot, sc->sc_ioh, VIRTIO_MMIO_QUEUE_ALIGN,
> PAGE_SIZE);
> - bus_space_write_4(sc->sc_iot, sc->sc_ioh, VIRTIO_MMIO_QUEUE_PFN, addr);
> + bus_space_write_4(sc->sc_iot, sc->sc_ioh, VIRTIO_MMIO_QUEUE_PFN,
> + addr / VIRTIO_PAGE_SIZE);
> }
>
> void
> diff --git a/sys/dev/pci/virtio_pci.c b/sys/dev/pci/virtio_pci.c
> index cce99e3d720..d1ab7481df6 100644
> --- a/sys/dev/pci/virtio_pci.c
> +++ b/sys/dev/pci/virtio_pci.c
> @@ -64,7 +64,7 @@ voidvirtio_pci_write_device_config_2(struct
> virtio_softc *, int, uint16_t);
> void virtio_pci_write_device_config_4(struct virtio_softc *, int,
> uint32_t);
> void virtio_pci_write_device_config_8(struct virtio_softc *, int,
> uint64_t);
> uint16_t virtio_pci_read_queue_size(struct virtio_softc *, uint16_t);
> -void virtio_pci_setup_queue(struct virtio_softc *, uint16_t,
> uint32_t);
> +void virtio_pci_setup_queue(struct virtio_softc *, struct virtqueue
> *, uint64_t);
> void virtio_pci_set_status(struct virtio_softc *, int);
> uint32_t virtio_pci_negotiate_features(struct virtio_softc *, uint32_t,
> const struct virtio_feature_name
> *);
> @@ -134,13 +134,14 @@ virtio_pci_read_queue_size(struct virtio_softc *vsc,
> uint16_t idx)
> }
>
> void
> -virtio_pci_setup_queue(struct virtio_softc *vsc, uint16_t idx, uint32_t addr)
> +virtio_pci_setup_queue(struct virtio_softc *vsc, struct virtqueue *vq,
> +uint64_t addr)
> {
> struct virtio_pci_softc *sc = (struct virtio_pci_softc *)vsc;
> bus_space_write_2(sc->sc_iot, sc->sc_ioh, VIRTIO_CONFIG_QUEUE_SELECT,
> - idx);
> + vq->vq_index);
> bus_space_write_4(sc->sc_iot, sc->sc_ioh, VIRTIO_CONFIG_QUEUE_ADDRESS,
> - addr);
> + addr / VIRTIO_PAGE_SIZE);
>
> /*
>* This path is only executed if this function is called after
> @@ -150,7 +151,7 @@ virtio_pci_setup_queue(struct virtio_softc *vsc, uint16_t
> idx, uint32_t addr)
> if (sc->sc_irq_type != IRQ_NO_MSIX) {
> int vec = 1;
> if (sc->sc_irq_type == IRQ_MSIX_PER_VQ)
> -vec += idx;
> +vec += vq->vq_index;
> bus_space_write_2(sc->sc_iot, sc->sc_ioh,
> VIRTIO_MSI_QUEUE_VECTOR, vec);
> }
> diff --git a/sys/dev/pv/virtio.c b/sys/dev/pv/virtio.c
> index e6cd1ace5aa..be81878642f 100644
> --- a/sys/dev/pv/virtio.c
> +++ b/sys/dev/pv/virtio.c
> @@ -154,8 +154,7 @@ virtio_reinit_start(struct virtio_softc *sc)
> sc->sc_dev.dv_xname, vq->vq_index);
> }
>
cd9660: unused field in iso_node
Hi,
The i_lockf field in `struct iso_node' looks unused since the initial
import. Also, vop_advlock is mapped to eopnotsupp() in cd9660_vops.
Comments? OK?
Index: isofs/cd9660/cd9660_node.h
===
RCS file: /cvs/src/sys/isofs/cd9660/cd9660_node.h,v
retrieving revision 1.20
diff -u -p -r1.20 cd9660_node.h
--- isofs/cd9660/cd9660_node.h 19 Jun 2016 11:54:33 - 1.20
+++ isofs/cd9660/cd9660_node.h 18 Jan 2019 14:09:58 -
@@ -61,7 +61,6 @@ struct iso_node {
cdino_t i_number; /* the identity of the inode */
/* we use the actual starting block of the file
*/
struct iso_mnt *i_mnt; /* filesystem associated with this inode */
- struct lockf *i_lockf; /* head of byte-level lock list */
doff_t i_endoff; /* end of useful stuff in directory */
doff_t i_diroff; /* offset in dir, where we found last entry */
doff_t i_offset; /* offset of free space in directory */
[PATCH 2/7] virtio: Prepare for 64 feature bits
virtio 1.0 supports an arbitrary number of feature bits. However, so far
no more than 64 are used (compared to 32 in virtio 0.9). Adjust data
types to support 64 feature bits.
Later, we may want to use bitmaps and setbit(), ... to support even more
feature bits.
---
sys/dev/fdt/virtio_mmio.c | 8
sys/dev/pci/virtio_pci.c | 8
sys/dev/pv/if_vio.c | 40 +++
sys/dev/pv/vioblk.c | 2 +-
sys/dev/pv/vioblkreg.h| 18 +-
sys/dev/pv/viocon.c | 6 +++---
sys/dev/pv/viomb.c| 4 ++--
sys/dev/pv/vioscsireg.h | 4 ++--
sys/dev/pv/virtio.c | 6 +++---
sys/dev/pv/virtioreg.h| 10 +-
sys/dev/pv/virtiovar.h| 6 +++---
sys/dev/pv/vmmci.c| 8
12 files changed, 60 insertions(+), 60 deletions(-)
diff --git a/sys/dev/fdt/virtio_mmio.c b/sys/dev/fdt/virtio_mmio.c
index 3a74e647c19..2d7a131125d 100644
--- a/sys/dev/fdt/virtio_mmio.c
+++ b/sys/dev/fdt/virtio_mmio.c
@@ -91,7 +91,7 @@ void virtio_mmio_write_device_config_8(struct
virtio_softc *, int, uint64_t);
uint16_t virtio_mmio_read_queue_size(struct virtio_softc *, uint16_t);
void virtio_mmio_setup_queue(struct virtio_softc *, struct virtqueue
*, uint64_t);
void virtio_mmio_set_status(struct virtio_softc *, int);
-uint32_t virtio_mmio_negotiate_features(struct virtio_softc *, uint32_t,
+uint64_t virtio_mmio_negotiate_features(struct virtio_softc *, uint64_t,
const struct virtio_feature_name
*);
intvirtio_mmio_intr(void *);
@@ -300,12 +300,12 @@ virtio_mmio_detach(struct device *self, int flags)
* Prints available / negotiated features if guest_feature_names != NULL and
* VIRTIO_DEBUG is 1
*/
-uint32_t
-virtio_mmio_negotiate_features(struct virtio_softc *vsc, uint32_t
guest_features,
+uint64_t
+virtio_mmio_negotiate_features(struct virtio_softc *vsc, uint64_t
guest_features,
const struct virtio_feature_name *guest_feature_names)
{
struct virtio_mmio_softc *sc = (struct virtio_mmio_softc *)vsc;
- uint32_t host, neg;
+ uint64_t host, neg;
/*
* indirect descriptors can be switched off by setting bit 1 in the
diff --git a/sys/dev/pci/virtio_pci.c b/sys/dev/pci/virtio_pci.c
index d1ab7481df6..97e5607a55e 100644
--- a/sys/dev/pci/virtio_pci.c
+++ b/sys/dev/pci/virtio_pci.c
@@ -66,7 +66,7 @@ void virtio_pci_write_device_config_8(struct
virtio_softc *, int, uint64_t);
uint16_t virtio_pci_read_queue_size(struct virtio_softc *, uint16_t);
void virtio_pci_setup_queue(struct virtio_softc *, struct virtqueue
*, uint64_t);
void virtio_pci_set_status(struct virtio_softc *, int);
-uint32_t virtio_pci_negotiate_features(struct virtio_softc *, uint32_t,
+uint64_t virtio_pci_negotiate_features(struct virtio_softc *, uint64_t,
const struct virtio_feature_name
*);
intvirtio_pci_msix_establish(struct virtio_pci_softc *, struct
pci_attach_args *, int, int (*)(void *), void *);
intvirtio_pci_setup_msix(struct virtio_pci_softc *, struct
pci_attach_args *, int);
@@ -317,12 +317,12 @@ virtio_pci_detach(struct device *self, int flags)
* Prints available / negotiated features if guest_feature_names != NULL and
* VIRTIO_DEBUG is 1
*/
-uint32_t
-virtio_pci_negotiate_features(struct virtio_softc *vsc, uint32_t
guest_features,
+uint64_t
+virtio_pci_negotiate_features(struct virtio_softc *vsc, uint64_t
guest_features,
const struct virtio_feature_name *guest_feature_names)
{
struct virtio_pci_softc *sc = (struct virtio_pci_softc *)vsc;
- uint32_t host, neg;
+ uint64_t host, neg;
/*
* indirect descriptors can be switched off by setting bit 1 in the
diff --git a/sys/dev/pv/if_vio.c b/sys/dev/pv/if_vio.c
index d79935ca512..a4cd80af62d 100644
--- a/sys/dev/pv/if_vio.c
+++ b/sys/dev/pv/if_vio.c
@@ -68,25 +68,25 @@
#define VIRTIO_NET_CONFIG_STATUS 6 /* 16bit */
/* Feature bits */
-#define VIRTIO_NET_F_CSUM (1<<0)
-#define VIRTIO_NET_F_GUEST_CSUM(1<<1)
-#define VIRTIO_NET_F_MAC (1<<5)
-#define VIRTIO_NET_F_GSO (1<<6)
-#define VIRTIO_NET_F_GUEST_TSO4(1<<7)
-#define VIRTIO_NET_F_GUEST_TSO6(1<<8)
-#define VIRTIO_NET_F_GUEST_ECN (1<<9)
-#define VIRTIO_NET_F_GUEST_UFO (1<<10)
-#define VIRTIO_NET_F_HOST_TSO4 (1<<11)
-#define VIRTIO_NET_F_HOST_TSO6 (1<<12)
-#define VIRTIO_NET_F_HOST_ECN (1<<13)
-#define VIRTIO_NET_F_HOST_UFO (1<<14)
-#define VIRTIO_NET_F_MRG_RXBUF (1<<15)
-#define VIRTIO_NET_F_STATUS(1<<16)
-#define VIRTIO_NET_F_CTRL_VQ (1<<17)
-#define VIRTIO_NET_F_CTRL_RX (1<<18)
-#define VIRTIO_NET_F_CTRL_VLAN (1<<19)
[PATCH 6/7] Rework virtio_negotiate_features()
Add a sc_driver_features field that is automatically used by
virtio_negotiate_features() and during reinit.
Make virtio_negotiate_features() return an error code. Virtio 1.0 has a
special status bit for feature negotiation that means that negotiation
can fail. Make virtio_negotiate_features() return an error code instead
of the features.
Make virtio_reinit_start() automatically call
virtio_negotiate_features().
Add a convenience function virtio_has_feature() to make checking bits
easier.
Add an error check in viomb for virtio_negotiate_features because it has
some feature bits that may cause negotiation to fail. More error
checking in the child drivers is still missing.
---
sys/dev/fdt/virtio_mmio.c | 38 ++
sys/dev/pci/virtio_pci.c | 39 +++
sys/dev/pv/if_vio.c | 38 +-
sys/dev/pv/vioblk.c | 25 +
sys/dev/pv/viocon.c | 6 +++---
sys/dev/pv/viomb.c| 9 +
sys/dev/pv/viornd.c | 2 +-
sys/dev/pv/vioscsi.c | 2 +-
sys/dev/pv/virtio.c | 13 +++--
sys/dev/pv/virtiovar.h| 15 ---
sys/dev/pv/vmmci.c| 13 ++---
11 files changed, 110 insertions(+), 90 deletions(-)
diff --git a/sys/dev/fdt/virtio_mmio.c b/sys/dev/fdt/virtio_mmio.c
index 2d7a131125d..6e09fb0e76f 100644
--- a/sys/dev/fdt/virtio_mmio.c
+++ b/sys/dev/fdt/virtio_mmio.c
@@ -91,8 +91,8 @@ void virtio_mmio_write_device_config_8(struct
virtio_softc *, int, uint64_t);
uint16_t virtio_mmio_read_queue_size(struct virtio_softc *, uint16_t);
void virtio_mmio_setup_queue(struct virtio_softc *, struct virtqueue
*, uint64_t);
void virtio_mmio_set_status(struct virtio_softc *, int);
-uint64_t virtio_mmio_negotiate_features(struct virtio_softc *, uint64_t,
- const struct virtio_feature_name
*);
+intvirtio_mmio_negotiate_features(struct virtio_softc *,
+const struct virtio_feature_name *);
intvirtio_mmio_intr(void *);
struct virtio_mmio_softc {
@@ -300,28 +300,42 @@ virtio_mmio_detach(struct device *self, int flags)
* Prints available / negotiated features if guest_feature_names != NULL and
* VIRTIO_DEBUG is 1
*/
-uint64_t
-virtio_mmio_negotiate_features(struct virtio_softc *vsc, uint64_t
guest_features,
- const struct virtio_feature_name *guest_feature_names)
+int
+virtio_mmio_negotiate_features(struct virtio_softc *vsc,
+const struct virtio_feature_name *guest_feature_names)
{
struct virtio_mmio_softc *sc = (struct virtio_mmio_softc *)vsc;
uint64_t host, neg;
+ vsc->sc_active_features = 0;
+
/*
-* indirect descriptors can be switched off by setting bit 1 in the
-* driver flags, see config(8)
+* We enable indirect descriptors by default. They can be switched
+* off by setting bit 1 in the driver flags, see config(8).
*/
if (!(vsc->sc_dev.dv_cfdata->cf_flags & VIRTIO_CF_NO_INDIRECT) &&
!(vsc->sc_child->dv_cfdata->cf_flags & VIRTIO_CF_NO_INDIRECT)) {
- guest_features |= VIRTIO_F_RING_INDIRECT_DESC;
- } else {
+ vsc->sc_driver_features |= VIRTIO_F_RING_INDIRECT_DESC;
+ } else if (guest_feature_names != NULL) {
printf("RingIndirectDesc disabled by UKC\n");
}
+ /*
+* The driver must add VIRTIO_F_RING_EVENT_IDX if it supports it.
+* If it did, check if it is disabled by bit 2 in the driver flags.
+*/
+ if ((vsc->sc_driver_features & VIRTIO_F_RING_EVENT_IDX) &&
+ ((vsc->sc_dev.dv_cfdata->cf_flags & VIRTIO_CF_NO_EVENT_IDX) ||
+ (vsc->sc_child->dv_cfdata->cf_flags & VIRTIO_CF_NO_EVENT_IDX))) {
+ if (guest_feature_names != NULL)
+ printf(" RingEventIdx disabled by UKC");
+ vsc->sc_driver_features &= ~(VIRTIO_F_RING_EVENT_IDX);
+ }
+
bus_space_write_4(sc->sc_iot, sc->sc_ioh,
VIRTIO_MMIO_HOST_FEATURES_SEL, 0);
host = bus_space_read_4(sc->sc_iot, sc->sc_ioh,
VIRTIO_MMIO_HOST_FEATURES);
- neg = host & guest_features;
+ neg = host & vsc->sc_driver_features;
#if VIRTIO_DEBUG
if (guest_feature_names)
virtio_log_features(host, neg, guest_feature_names);
@@ -330,13 +344,13 @@ virtio_mmio_negotiate_features(struct virtio_softc *vsc,
uint64_t guest_features
VIRTIO_MMIO_GUEST_FEATURES_SEL, 0);
bus_space_write_4(sc->sc_iot, sc->sc_ioh,
VIRTIO_MMIO_GUEST_FEATURES, neg);
- vsc->sc_features = neg;
+ vsc->sc_active_features = neg;
if (neg & VIRTIO_F_RING_INDIRECT_DESC)
vsc->sc_indirect = 1;
else
vsc->sc_indirect = 0;
- return neg;
+
[PATCH 7/7] Support virtio 1.0 for virtio_pci
virtio 1.0 for virtio_mmio it not yet implemented, but 0.9 devices
continue to work.
---
share/man/man4/virtio.4 | 11 +-
sys/dev/pci/virtio_pci.c| 518 +++-
sys/dev/pci/virtio_pcireg.h | 57
sys/dev/pv/if_vio.c | 9 +-
sys/dev/pv/virtiovar.h | 5 +
5 files changed, 531 insertions(+), 69 deletions(-)
diff --git a/share/man/man4/virtio.4 b/share/man/man4/virtio.4
index 5e60e2e0c32..dd9d4551715 100644
--- a/share/man/man4/virtio.4
+++ b/share/man/man4/virtio.4
@@ -22,7 +22,7 @@
.Nd VirtIO support driver
.Sh SYNOPSIS
.Cd "virtio* at fdt?"
-.Cd "virtio* at pci?"
+.Cd "virtio* at pci? flags 0x00"
.Sh DESCRIPTION
The
.Nm
@@ -56,7 +56,14 @@ control interface
The
.Nm
driver conforms to the virtio 0.9.5 specification.
-The virtio 1.0 standard is not supported, yet.
+The virtio 1.0 standard is only supported for pci devices.
+.Pp
+By default 0.9 is preferred over 1.0.
+This can be changed by setting the bit 0x4 in the flags.
+.Pp
+Setting the bit 0x8 in the flags disables the 1.0 support completely.
+.Sh BUGS
+.Nm Big-endian architectures are not yet supported.
.Sh SEE ALSO
.Xr intro 4
.Sh HISTORY
diff --git a/sys/dev/pci/virtio_pci.c b/sys/dev/pci/virtio_pci.c
index f370f513e85..591da61290a 100644
--- a/sys/dev/pci/virtio_pci.c
+++ b/sys/dev/pci/virtio_pci.c
@@ -35,11 +35,16 @@
#include
#include
#include
+#include
#include
#include
#include
+#define DNPRINTF(n,x...) \
+do { if (VIRTIO_DEBUG >= n) printf(x); } while(0)
+
+
/*
* XXX: Before being used on big endian arches, the access to config registers
* XXX: needs to be reviewed/fixed. The non-device specific registers are
@@ -52,6 +57,8 @@ struct virtio_pci_softc;
intvirtio_pci_match(struct device *, void *, void *);
void virtio_pci_attach(struct device *, struct device *, void *);
+intvirtio_pci_attach_09(struct virtio_pci_softc *sc, struct
pci_attach_args *pa);
+intvirtio_pci_attach_10(struct virtio_pci_softc *sc, struct
pci_attach_args *pa);
intvirtio_pci_detach(struct device *, int);
void virtio_pci_kick(struct virtio_softc *, uint16_t);
@@ -67,8 +74,8 @@ void virtio_pci_write_device_config_8(struct
virtio_softc *, int, uint64_t);
uint16_t virtio_pci_read_queue_size(struct virtio_softc *, uint16_t);
void virtio_pci_setup_queue(struct virtio_softc *, struct virtqueue
*, uint64_t);
void virtio_pci_set_status(struct virtio_softc *, int);
-intvirtio_pci_negotiate_features(struct virtio_softc *,
-const struct virtio_feature_name *);
+intvirtio_pci_negotiate_features(struct virtio_softc *, const
struct virtio_feature_name *);
+intvirtio_pci_negotiate_features_10(struct virtio_softc *, const
struct virtio_feature_name *);
void virtio_pci_set_msix_queue_vector(struct virtio_pci_softc *,
uint32_t, uint16_t);
void virtio_pci_set_msix_config_vector(struct virtio_pci_softc *,
uint16_t);
intvirtio_pci_msix_establish(struct virtio_pci_softc *, struct
pci_attach_args *, int, int (*)(void *), void *);
@@ -80,6 +87,10 @@ int virtio_pci_legacy_intr_mpsafe(void *);
intvirtio_pci_config_intr(void *);
intvirtio_pci_queue_intr(void *);
intvirtio_pci_shared_queue_intr(void *);
+intvirtio_pci_find_cap(struct virtio_pci_softc *sc, int cfg_type,
void *buf, int buflen);
+#if VIRTIO_DEBUG
+void virtio_pci_dump_caps(struct virtio_pci_softc *sc);
+#endif
enum irq_type {
IRQ_NO_MSIX,
@@ -96,9 +107,14 @@ struct virtio_pci_softc {
bus_space_handle_t sc_ioh;
bus_size_t sc_iosize;
+ bus_space_tag_t sc_bars_iot[4];
+ bus_space_handle_t sc_bars_ioh[4];
+ bus_size_t sc_bars_iosize[4];
+
bus_space_tag_t sc_notify_iot;
bus_space_handle_t sc_notify_ioh;
bus_size_t sc_notify_iosize;
+ unsigned intsc_notify_off_multiplier;
bus_space_tag_t sc_devcfg_iot;
bus_space_handle_t sc_devcfg_ioh;
@@ -145,14 +161,69 @@ struct virtio_ops virtio_pci_ops = {
virtio_pci_poll_intr,
};
+static inline
+uint64_t _cread(struct virtio_pci_softc *sc, unsigned off, unsigned size)
+{
+ uint64_t val;
+ switch (size) {
+ case 1:
+ val = bus_space_read_1(sc->sc_iot, sc->sc_ioh, off);
+ break;
+ case 2:
+ val = bus_space_read_2(sc->sc_iot, sc->sc_ioh, off);
+ break;
+ case 4:
+ val = bus_space_read_4(sc->sc_iot, sc->sc_ioh, off);
+ break;
+ case 8:
+ val = bus_space_read_8(sc->sc_iot, sc->sc_ioh, off);
+ break;
+ }
+ return val;
+}
+
+#define CREAD(sc, memb) _cread(sc, offsetof(struct
[PATCH 4/7] virtio_pci: Split bus space handles
In virtio_pci 1.0, different parts of the register set may be located in
different BARs. Use subregions to make the access independent of the
virtio version.
---
sys/dev/pci/virtio_pci.c | 114 +++
1 file changed, 80 insertions(+), 34 deletions(-)
diff --git a/sys/dev/pci/virtio_pci.c b/sys/dev/pci/virtio_pci.c
index 97e5607a55e..d69db1968d0 100644
--- a/sys/dev/pci/virtio_pci.c
+++ b/sys/dev/pci/virtio_pci.c
@@ -55,6 +55,7 @@ void virtio_pci_attach(struct device *, struct
device *, void *);
intvirtio_pci_detach(struct device *, int);
void virtio_pci_kick(struct virtio_softc *, uint16_t);
+intvirtio_pci_adjust_config_region(struct virtio_pci_softc *);
uint8_tvirtio_pci_read_device_config_1(struct virtio_softc *,
int);
uint16_t virtio_pci_read_device_config_2(struct virtio_softc *, int);
uint32_t virtio_pci_read_device_config_4(struct virtio_softc *, int);
@@ -87,14 +88,33 @@ enum irq_type {
struct virtio_pci_softc {
struct virtio_softc sc_sc;
pci_chipset_tag_t sc_pc;
+ pcitag_tsc_ptag;
bus_space_tag_t sc_iot;
bus_space_handle_t sc_ioh;
bus_size_t sc_iosize;
+ bus_space_tag_t sc_notify_iot;
+ bus_space_handle_t sc_notify_ioh;
+ bus_size_t sc_notify_iosize;
+
+ bus_space_tag_t sc_devcfg_iot;
+ bus_space_handle_t sc_devcfg_ioh;
+ bus_size_t sc_devcfg_iosize;
+ /*
+* With 0.9, the offset of the devcfg region in the io bar changes
+* depending on MSI-X being enabled or not.
+* With 1.0, this field is still used to remember if MSI-X is enabled
+* or not.
+*/
+ unsigned intsc_devcfg_offset;
+
+ bus_space_tag_t sc_isr_iot;
+ bus_space_handle_t sc_isr_ioh;
+ bus_size_t sc_isr_iosize;
+
void*sc_ih[MAX_MSIX_VECS];
- int sc_config_offset;
enum irq_type sc_irq_type;
};
@@ -213,9 +233,8 @@ virtio_pci_attach(struct device *parent, struct device
*self, void *aux)
vsc->sc_ops = _pci_ops;
sc->sc_pc = pc;
+ sc->sc_ptag = pa->pa_tag;
vsc->sc_dmat = pa->pa_dmat;
- sc->sc_config_offset = VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI;
- sc->sc_irq_type = IRQ_NO_MSIX;
/*
* For virtio, ignore normal MSI black/white-listing depending on the
@@ -229,11 +248,33 @@ virtio_pci_attach(struct device *parent, struct device
*self, void *aux)
return;
}
+ if (bus_space_subregion(sc->sc_iot, sc->sc_ioh,
+ VIRTIO_CONFIG_QUEUE_NOTIFY, 2, >sc_notify_ioh) != 0) {
+ printf("%s: can't map notify i/o space\n",
+ vsc->sc_dev.dv_xname);
+ return;
+ }
+ sc->sc_notify_iosize = 2;
+ sc->sc_notify_iot = sc->sc_iot;
+
+ if (bus_space_subregion(sc->sc_iot, sc->sc_ioh,
+ VIRTIO_CONFIG_ISR_STATUS, 1, >sc_isr_ioh) != 0) {
+ printf("%s: can't map isr i/o space\n",
+ vsc->sc_dev.dv_xname);
+ return;
+ }
+ sc->sc_isr_iosize = 1;
+ sc->sc_isr_iot = sc->sc_iot;
+
+ sc->sc_devcfg_offset = VIRTIO_CONFIG_DEVICE_CONFIG_NOMSI;
+ sc->sc_irq_type = IRQ_NO_MSIX;
+ if (virtio_pci_adjust_config_region(sc) != 0)
+ return;
+
virtio_device_reset(vsc);
virtio_pci_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_ACK);
virtio_pci_set_status(vsc, VIRTIO_CONFIG_DEVICE_STATUS_DRIVER);
- /* XXX: use softc as aux... */
vsc->sc_childdevid = id;
vsc->sc_child = NULL;
config_found(self, sc, NULL);
@@ -312,6 +353,20 @@ virtio_pci_detach(struct device *self, int flags)
return 0;
}
+int
+virtio_pci_adjust_config_region(struct virtio_pci_softc *sc)
+{
+ sc->sc_devcfg_iosize = sc->sc_iosize - sc->sc_devcfg_offset;
+ sc->sc_devcfg_iot = sc->sc_iot;
+ if (bus_space_subregion(sc->sc_iot, sc->sc_ioh, sc->sc_devcfg_offset,
+ sc->sc_devcfg_iosize, >sc_devcfg_ioh) != 0) {
+ printf("%s: can't map config i/o space\n",
+ sc->sc_sc.sc_dev.dv_xname);
+ return 1;
+ }
+ return 0;
+}
+
/*
* Feature negotiation.
* Prints available / negotiated features if guest_feature_names != NULL and
@@ -359,24 +414,21 @@ uint8_t
virtio_pci_read_device_config_1(struct virtio_softc *vsc, int index)
{
struct virtio_pci_softc *sc = (struct virtio_pci_softc *)vsc;
- return bus_space_read_1(sc->sc_iot, sc->sc_ioh,
- sc->sc_config_offset + index);
+ return bus_space_read_1(sc->sc_devcfg_iot, sc->sc_devcfg_ioh, index);
}
uint16_t
virtio_pci_read_device_config_2(struct virtio_softc *vsc, int
[PATCH 5/7] virtio_pci: Move msix vector config into functions
---
sys/dev/pci/virtio_pci.c | 38 --
1 file changed, 24 insertions(+), 14 deletions(-)
diff --git a/sys/dev/pci/virtio_pci.c b/sys/dev/pci/virtio_pci.c
index d69db1968d0..7be93684a68 100644
--- a/sys/dev/pci/virtio_pci.c
+++ b/sys/dev/pci/virtio_pci.c
@@ -69,6 +69,8 @@ void virtio_pci_setup_queue(struct virtio_softc *,
struct virtqueue *, uint64_t
void virtio_pci_set_status(struct virtio_softc *, int);
uint64_t virtio_pci_negotiate_features(struct virtio_softc *, uint64_t,
const struct virtio_feature_name
*);
+void virtio_pci_set_msix_queue_vector(struct virtio_pci_softc *,
uint32_t, uint16_t);
+void virtio_pci_set_msix_config_vector(struct virtio_pci_softc *,
uint16_t);
intvirtio_pci_msix_establish(struct virtio_pci_softc *, struct
pci_attach_args *, int, int (*)(void *), void *);
intvirtio_pci_setup_msix(struct virtio_pci_softc *, struct
pci_attach_args *, int);
void virtio_pci_free_irqs(struct virtio_pci_softc *);
@@ -503,6 +505,22 @@ virtio_pci_msix_establish(struct virtio_pci_softc *sc,
return 0;
}
+void
+virtio_pci_set_msix_queue_vector(struct virtio_pci_softc *sc, uint32_t idx,
uint16_t vector)
+{
+ bus_space_write_2(sc->sc_iot, sc->sc_ioh,
+ VIRTIO_CONFIG_QUEUE_SELECT, idx);
+ bus_space_write_2(sc->sc_iot, sc->sc_ioh,
+ VIRTIO_MSI_QUEUE_VECTOR, vector);
+}
+
+void
+virtio_pci_set_msix_config_vector(struct virtio_pci_softc *sc, uint16_t vector)
+{
+ bus_space_write_2(sc->sc_iot, sc->sc_ioh,
+ VIRTIO_MSI_CONFIG_VECTOR, vector);
+}
+
void
virtio_pci_free_irqs(struct virtio_pci_softc *sc)
{
@@ -511,10 +529,8 @@ virtio_pci_free_irqs(struct virtio_pci_softc *sc)
if (sc->sc_devcfg_offset == VIRTIO_CONFIG_DEVICE_CONFIG_MSI) {
for (i = 0; i < vsc->sc_nvqs; i++) {
- bus_space_write_2(sc->sc_iot, sc->sc_ioh,
- VIRTIO_CONFIG_QUEUE_SELECT, i);
- bus_space_write_2(sc->sc_iot, sc->sc_ioh,
- VIRTIO_MSI_QUEUE_VECTOR, VIRTIO_MSI_NO_VECTOR);
+ virtio_pci_set_msix_queue_vector(sc, i,
+ VIRTIO_MSI_NO_VECTOR);
}
}
@@ -540,6 +556,7 @@ virtio_pci_setup_msix(struct virtio_pci_softc *sc, struct
pci_attach_args *pa,
return 1;
sc->sc_devcfg_offset = VIRTIO_CONFIG_DEVICE_CONFIG_MSI;
virtio_pci_adjust_config_region(sc);
+ virtio_pci_set_msix_config_vector(sc, 0);
if (shared) {
if (virtio_pci_msix_establish(sc, pa, 1,
@@ -547,22 +564,15 @@ virtio_pci_setup_msix(struct virtio_pci_softc *sc, struct
pci_attach_args *pa,
goto fail;
}
- for (i = 0; i < vsc->sc_nvqs; i++) {
- bus_space_write_2(sc->sc_iot, sc->sc_ioh,
- VIRTIO_CONFIG_QUEUE_SELECT, i);
- bus_space_write_2(sc->sc_iot, sc->sc_ioh,
- VIRTIO_MSI_QUEUE_VECTOR, 1);
- }
+ for (i = 0; i < vsc->sc_nvqs; i++)
+ virtio_pci_set_msix_queue_vector(sc, i, 1);
} else {
for (i = 0; i <= vsc->sc_nvqs; i++) {
if (virtio_pci_msix_establish(sc, pa, i + 1,
virtio_pci_queue_intr, >sc_vqs[i])) {
goto fail;
}
- bus_space_write_2(sc->sc_iot, sc->sc_ioh,
- VIRTIO_CONFIG_QUEUE_SELECT, i);
- bus_space_write_2(sc->sc_iot, sc->sc_ioh,
- VIRTIO_MSI_QUEUE_VECTOR, i + 1);
+ virtio_pci_set_msix_queue_vector(sc, i, i + 1);
}
}
--
2.19.0
[PATCH 1/7] virtio: adjust virtio_setup_queue prototype for 1.0
Make it take an address instead of a PFN.
Pass the virtqueue pointer. In virtio 1.0, more information has to be
configured in the device. Also call virtio_setup_queue() after the
information has been filled in.
---
sys/dev/fdt/virtio_mmio.c | 11 +++
sys/dev/pci/virtio_pci.c | 11 ++-
sys/dev/pv/virtio.c | 11 ---
sys/dev/pv/virtiovar.h| 2 +-
4 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/sys/dev/fdt/virtio_mmio.c b/sys/dev/fdt/virtio_mmio.c
index 11867a43173..3a74e647c19 100644
--- a/sys/dev/fdt/virtio_mmio.c
+++ b/sys/dev/fdt/virtio_mmio.c
@@ -89,7 +89,7 @@ void virtio_mmio_write_device_config_2(struct
virtio_softc *, int, uint16_t);
void virtio_mmio_write_device_config_4(struct virtio_softc *, int,
uint32_t);
void virtio_mmio_write_device_config_8(struct virtio_softc *, int,
uint64_t);
uint16_t virtio_mmio_read_queue_size(struct virtio_softc *, uint16_t);
-void virtio_mmio_setup_queue(struct virtio_softc *, uint16_t,
uint32_t);
+void virtio_mmio_setup_queue(struct virtio_softc *, struct virtqueue
*, uint64_t);
void virtio_mmio_set_status(struct virtio_softc *, int);
uint32_t virtio_mmio_negotiate_features(struct virtio_softc *, uint32_t,
const struct virtio_feature_name
*);
@@ -151,15 +151,18 @@ virtio_mmio_read_queue_size(struct virtio_softc *vsc,
uint16_t idx)
}
void
-virtio_mmio_setup_queue(struct virtio_softc *vsc, uint16_t idx, uint32_t addr)
+virtio_mmio_setup_queue(struct virtio_softc *vsc, struct virtqueue *vq,
+uint64_t addr)
{
struct virtio_mmio_softc *sc = (struct virtio_mmio_softc *)vsc;
- bus_space_write_4(sc->sc_iot, sc->sc_ioh, VIRTIO_MMIO_QUEUE_SEL, idx);
+ bus_space_write_4(sc->sc_iot, sc->sc_ioh, VIRTIO_MMIO_QUEUE_SEL,
+ vq->vq_index);
bus_space_write_4(sc->sc_iot, sc->sc_ioh, VIRTIO_MMIO_QUEUE_NUM,
bus_space_read_4(sc->sc_iot, sc->sc_ioh,
VIRTIO_MMIO_QUEUE_NUM_MAX));
bus_space_write_4(sc->sc_iot, sc->sc_ioh, VIRTIO_MMIO_QUEUE_ALIGN,
PAGE_SIZE);
- bus_space_write_4(sc->sc_iot, sc->sc_ioh, VIRTIO_MMIO_QUEUE_PFN, addr);
+ bus_space_write_4(sc->sc_iot, sc->sc_ioh, VIRTIO_MMIO_QUEUE_PFN,
+ addr / VIRTIO_PAGE_SIZE);
}
void
diff --git a/sys/dev/pci/virtio_pci.c b/sys/dev/pci/virtio_pci.c
index cce99e3d720..d1ab7481df6 100644
--- a/sys/dev/pci/virtio_pci.c
+++ b/sys/dev/pci/virtio_pci.c
@@ -64,7 +64,7 @@ void virtio_pci_write_device_config_2(struct
virtio_softc *, int, uint16_t);
void virtio_pci_write_device_config_4(struct virtio_softc *, int,
uint32_t);
void virtio_pci_write_device_config_8(struct virtio_softc *, int,
uint64_t);
uint16_t virtio_pci_read_queue_size(struct virtio_softc *, uint16_t);
-void virtio_pci_setup_queue(struct virtio_softc *, uint16_t,
uint32_t);
+void virtio_pci_setup_queue(struct virtio_softc *, struct virtqueue
*, uint64_t);
void virtio_pci_set_status(struct virtio_softc *, int);
uint32_t virtio_pci_negotiate_features(struct virtio_softc *, uint32_t,
const struct virtio_feature_name
*);
@@ -134,13 +134,14 @@ virtio_pci_read_queue_size(struct virtio_softc *vsc,
uint16_t idx)
}
void
-virtio_pci_setup_queue(struct virtio_softc *vsc, uint16_t idx, uint32_t addr)
+virtio_pci_setup_queue(struct virtio_softc *vsc, struct virtqueue *vq,
+uint64_t addr)
{
struct virtio_pci_softc *sc = (struct virtio_pci_softc *)vsc;
bus_space_write_2(sc->sc_iot, sc->sc_ioh, VIRTIO_CONFIG_QUEUE_SELECT,
- idx);
+ vq->vq_index);
bus_space_write_4(sc->sc_iot, sc->sc_ioh, VIRTIO_CONFIG_QUEUE_ADDRESS,
- addr);
+ addr / VIRTIO_PAGE_SIZE);
/*
* This path is only executed if this function is called after
@@ -150,7 +151,7 @@ virtio_pci_setup_queue(struct virtio_softc *vsc, uint16_t
idx, uint32_t addr)
if (sc->sc_irq_type != IRQ_NO_MSIX) {
int vec = 1;
if (sc->sc_irq_type == IRQ_MSIX_PER_VQ)
- vec += idx;
+ vec += vq->vq_index;
bus_space_write_2(sc->sc_iot, sc->sc_ioh,
VIRTIO_MSI_QUEUE_VECTOR, vec);
}
diff --git a/sys/dev/pv/virtio.c b/sys/dev/pv/virtio.c
index e6cd1ace5aa..be81878642f 100644
--- a/sys/dev/pv/virtio.c
+++ b/sys/dev/pv/virtio.c
@@ -154,8 +154,7 @@ virtio_reinit_start(struct virtio_softc *sc)
sc->sc_dev.dv_xname, vq->vq_index);
}
virtio_init_vq(sc, vq, 1);
- virtio_setup_queue(sc, vq->vq_index,
- vq->vq_dmamap->dm_segs[0].ds_addr / VIRTIO_PAGE_SIZE);
+ virtio_setup_queue(sc, vq, vq->vq_dmamap->dm_segs[0].ds_addr);
}
}
@@ -345,9 +344,6 @@
[PATCH 0/7] Virtio 1.0 in the kernel
Hi, here comes the split up diff for virtio 1.0 support. Compared to the previous diff, I have omitted some changes in how the feature bits and negotiation works. This also means that the feature bit defines in the headers stay the same and vmd is not affected. I have tested that virtio_mmio on arm64 still works. Cheers, Stefan
[PATCH 3/7] virtio: Add a few feature bit defines and names
---
sys/dev/pv/if_vio.c| 84 +++---
sys/dev/pv/vioblk.c| 3 ++
sys/dev/pv/vioblkreg.h | 21 ++-
sys/dev/pv/virtio.c| 1 +
4 files changed, 62 insertions(+), 47 deletions(-)
diff --git a/sys/dev/pv/if_vio.c b/sys/dev/pv/if_vio.c
index a4cd80af62d..bfc7cfd1ddc 100644
--- a/sys/dev/pv/if_vio.c
+++ b/sys/dev/pv/if_vio.c
@@ -68,25 +68,29 @@
#define VIRTIO_NET_CONFIG_STATUS 6 /* 16bit */
/* Feature bits */
-#define VIRTIO_NET_F_CSUM (1ULL<<0)
-#define VIRTIO_NET_F_GUEST_CSUM(1ULL<<1)
-#define VIRTIO_NET_F_MAC (1ULL<<5)
-#define VIRTIO_NET_F_GSO (1ULL<<6)
-#define VIRTIO_NET_F_GUEST_TSO4(1ULL<<7)
-#define VIRTIO_NET_F_GUEST_TSO6(1ULL<<8)
-#define VIRTIO_NET_F_GUEST_ECN (1ULL<<9)
-#define VIRTIO_NET_F_GUEST_UFO (1ULL<<10)
-#define VIRTIO_NET_F_HOST_TSO4 (1ULL<<11)
-#define VIRTIO_NET_F_HOST_TSO6 (1ULL<<12)
-#define VIRTIO_NET_F_HOST_ECN (1ULL<<13)
-#define VIRTIO_NET_F_HOST_UFO (1ULL<<14)
-#define VIRTIO_NET_F_MRG_RXBUF (1ULL<<15)
-#define VIRTIO_NET_F_STATUS(1ULL<<16)
-#define VIRTIO_NET_F_CTRL_VQ (1ULL<<17)
-#define VIRTIO_NET_F_CTRL_RX (1ULL<<18)
-#define VIRTIO_NET_F_CTRL_VLAN (1ULL<<19)
-#define VIRTIO_NET_F_CTRL_RX_EXTRA (1ULL<<20)
-#define VIRTIO_NET_F_GUEST_ANNOUNCE(1ULL<<21)
+#define VIRTIO_NET_F_CSUM (1ULL<<0)
+#define VIRTIO_NET_F_GUEST_CSUM(1ULL<<1)
+#define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS(1ULL<<2)
+#define VIRTIO_NET_F_MTU(1ULL<<3)
+#define VIRTIO_NET_F_MAC (1ULL<<5)
+#define VIRTIO_NET_F_GSO (1ULL<<6)
+#define VIRTIO_NET_F_GUEST_TSO4(1ULL<<7)
+#define VIRTIO_NET_F_GUEST_TSO6(1ULL<<8)
+#define VIRTIO_NET_F_GUEST_ECN (1ULL<<9)
+#define VIRTIO_NET_F_GUEST_UFO (1ULL<<10)
+#define VIRTIO_NET_F_HOST_TSO4 (1ULL<<11)
+#define VIRTIO_NET_F_HOST_TSO6 (1ULL<<12)
+#define VIRTIO_NET_F_HOST_ECN (1ULL<<13)
+#define VIRTIO_NET_F_HOST_UFO (1ULL<<14)
+#define VIRTIO_NET_F_MRG_RXBUF (1ULL<<15)
+#define VIRTIO_NET_F_STATUS(1ULL<<16)
+#define VIRTIO_NET_F_CTRL_VQ (1ULL<<17)
+#define VIRTIO_NET_F_CTRL_RX (1ULL<<18)
+#define VIRTIO_NET_F_CTRL_VLAN (1ULL<<19)
+#define VIRTIO_NET_F_CTRL_RX_EXTRA (1ULL<<20)
+#define VIRTIO_NET_F_GUEST_ANNOUNCE(1ULL<<21)
+#define VIRTIO_NET_F_MQ(1ULL<<22)
+#define VIRTIO_NET_F_CTRL_MAC_ADDR (1ULL<<23)
/*
* Config(8) flags. The lowest byte is reserved for generic virtio stuff.
@@ -97,25 +101,29 @@
static const struct virtio_feature_name virtio_net_feature_names[] = {
#if VIRTIO_DEBUG
- { VIRTIO_NET_F_CSUM,"CSum" },
- { VIRTIO_NET_F_GUEST_CSUM, "GuestCSum" },
- { VIRTIO_NET_F_MAC, "MAC" },
- { VIRTIO_NET_F_GSO, "GSO" },
- { VIRTIO_NET_F_GUEST_TSO4, "GuestTSO4" },
- { VIRTIO_NET_F_GUEST_TSO6, "GuestTSO6" },
- { VIRTIO_NET_F_GUEST_ECN, "GuestECN" },
- { VIRTIO_NET_F_GUEST_UFO, "GuestUFO" },
- { VIRTIO_NET_F_HOST_TSO4, "HostTSO4" },
- { VIRTIO_NET_F_HOST_TSO6, "HostTSO6" },
- { VIRTIO_NET_F_HOST_ECN,"HostECN" },
- { VIRTIO_NET_F_HOST_UFO,"HostUFO" },
- { VIRTIO_NET_F_MRG_RXBUF, "MrgRXBuf" },
- { VIRTIO_NET_F_STATUS, "Status" },
- { VIRTIO_NET_F_CTRL_VQ, "CtrlVQ" },
- { VIRTIO_NET_F_CTRL_RX, "CtrlRX" },
- { VIRTIO_NET_F_CTRL_VLAN, "CtrlVLAN" },
- { VIRTIO_NET_F_CTRL_RX_EXTRA, "CtrlRXExtra" },
- { VIRTIO_NET_F_GUEST_ANNOUNCE, "GuestAnnounce" },
+ { VIRTIO_NET_F_CSUM,"CSum" },
+ { VIRTIO_NET_F_GUEST_CSUM, "GuestCSum" },
+ { VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, "CtrlGuestOffl" },
+ { VIRTIO_NET_F_MTU, "MTU", },
+ { VIRTIO_NET_F_MAC, "MAC" },
+ { VIRTIO_NET_F_GSO, "GSO" },
+ { VIRTIO_NET_F_GUEST_TSO4, "GuestTSO4" },
+ { VIRTIO_NET_F_GUEST_TSO6, "GuestTSO6" },
+ { VIRTIO_NET_F_GUEST_ECN, "GuestECN" },
+ { VIRTIO_NET_F_GUEST_UFO, "GuestUFO" },
+ { VIRTIO_NET_F_HOST_TSO4, "HostTSO4" },
+ { VIRTIO_NET_F_HOST_TSO6, "HostTSO6" },
+ { VIRTIO_NET_F_HOST_ECN,"HostECN" },
+ { VIRTIO_NET_F_HOST_UFO,"HostUFO" },
+ { VIRTIO_NET_F_MRG_RXBUF, "MrgRXBuf" },
+ {
Re: [PATCH] systat reports inaccurate statistics for disk i/o speed
On 2019-01-19 15:00:20, Otto Moerbeek wrote: > On Sat, Jan 19, 2019 at 09:30:12PM +0900, Bryan Linton wrote: > > > Hello tech@, > > > > I'd appreciate it if someone could review both this patch and my > > analysis. > > > > [...] > > > > ADDENDUM > > > > > > I'm curious why iostat.c uses cur.dk_*[dn] directly whereas > > vmstat.c adds 0.5 to it. > > rsum += cur.dk_rbytes[dn] / etime; > > rsum is a double. When adding a double to a double, there's no > rounding or truncation going on. > > > vs. > > words = (cur.dk_rbytes[dn] + cur.dk_wbytes[dn]) / 1024.0; > > putintmk((int)(words/etime + 0.5), DISKROW + 3, c, 5); > >^^ > > When converting a double (or float) to an int, truncation happens. So > add 0.5 to turn it into rounding. > Ah, OK. I get it now. > > Also, there are several casts in lines like > > putint((int)((float)cur.dk_seek[dn]/etime+0.5), DISKROW + 1, c, 5) > > > > I understand why the value would need to be cast to an (int), > > since putint() is expecting an int, but since etime is already a > > (double), wouldn't the C language's automatic type-promotion rules > > automatically promote everything to a (double) anyway? > > Those casts are indeed unneeded, > Thank you for taking the time to explain both of these! -- Bryan
Re: return value check in snprintf(3) example code
Hi Theo and Theo, Theo de Raadt wrote on Fri, Jan 18, 2019 at 08:06:32PM -0700: > Ted Unangst wrote: >> Theo Buehler wrote: >>> According to our documentation and all the standards I checked, >>> snprintf() returns a negative value on error, not necessarily -1. >>> This confused me quite a bit recently so I suggest to adjust the >>> example code as follows: >> I don't know. I guess it's technially correct, but there's theory >> and practice, and I don't think anybody will ever return -2 here. >> Everything in base is written to check for -1. >> >> In read.2 and write.2, we specially say that -1 is the correct value >> to check. It would be nice if the rules were consistent. >> (To that end, I'd consider it a standards bug that snprintf >> is underspecified. It should say -1.) > Yep. > > I dug into my email archives of July 1998. Chris Torek, Casper Dik and > I worked to convince everyone of the 4.4lite2 approach of returning "bytes > desired", and -1 only for the obscure errno-delivering cases. Since errno > is returned, the interface should return -1 (precisely). > > There are many thousands of lines of code checking for -1 (precisely), > and not checking for other negative numbers. Any library which returns > a different negative value will break many security-sensitive prorams. > > Please don't change our manual page. If anything, reach out to ANSI and > POSIX and ask them to seperate description of snprintf from fprintf, > and indicate snprintf returns precisely -1 in this circumstance. In that case, if it is considered important that checking != -1 is the right way and checking < 0 is bad style, the guarantee that the return value is always -1 in this case should be documented. And if we do that, then the STANDARDS section also needs an additional explanation. This has repeatedly caused confusion among developers in the past, and even more so among users, and there is an apparent contradiction between the RETURN VALUES and EXAMPLES sections. That should be fixed in one way or the other. See below for a patch to document our implementation more tightly. Frankly, from a documentation perspective, i prefer tb@'s patch: even if it is only theoretically safer, it is shorter and simpler and easier to understand without recommending something that is wrong or dangerous. Then again, maybe it might invite wild goose chases - people sending diffs to change the idiom in real code... Did i get the list of functions right that this applies to? OK for this version of the patch? Ingo Index: printf.3 === RCS file: /cvs/src/lib/libc/stdio/printf.3,v retrieving revision 1.79 diff -u -r1.79 printf.3 --- printf.316 Jan 2019 12:55:49 - 1.79 +++ printf.319 Jan 2019 14:15:48 - @@ -643,8 +643,15 @@ .Sh RETURN VALUES For all these functions if an output or encoding error occurs, a value less than 0 is returned. +In this case, +.Fn snprintf , +.Fn vsnprintf , +.Fn asprintf , +and +.Fn vasprintf +always return -1. .Pp -The +Otherwise, the .Fn printf , .Fn dprintf , .Fn fprintf , @@ -779,6 +786,17 @@ .Fn vdprintf functions conform to .St -p1003.1-2008 . +.Pp +Allowing +.Fn snprintf , +.Fn vsnprintf , +.Fn asprintf , +and +.Fn vasprintf +to return negative values other than -1 is a mistake in the standard. +In practice, no implementation is known to ever return other values, +and large amounts of existing code rely on the fact that exactly -1 +is returned. .Sh HISTORY The predecessors .Fn ftoa
Re: Virtio 1.0 for the kernel
On Saturday, 12 January 2019 01:49:09 CET Jonathan Gray wrote:
> On Fri, Jan 11, 2019 at 03:21:11PM -0800, William Ahern wrote:
> > On Fri, Jan 11, 2019 at 10:43:25AM +0100, Stefan Fritsch wrote:
> >
> >
> > > /* only used for sizeof, not actually allocated */
> > > extern struct virtio_pci_common_cfg ccfg;
> >
> >
> >
> > > #define CREAD(sc, memb) _cread(sc, \
> > >
> > > offsetof(struct virtio_pci_common_cfg, memb), sizeof(ccfg.memb))
> > >
> > > The compiler should optimize this to the same code as the complicated
> > > macro above. You think this variant is acceptable?
> >
> > Maybe I'm missing something, but these are the more idiomatic constructs
> >
> > sizeof ((struct virtio_pci_common_cfg *)0)->memb
> > sizeof ((struct virtio_pci_common_cfg){ 0 }).memb
>
> No, expanding the offsetof macro and avoiding __builtin_offsetof misses
> the point of it.
It allows to get rid of the "extern struct virtio_pci_common_cfg ccfg;"
though. That's an improvement.
Stefan
Re: pfctl: use mnemonic macros, terminate string with null char
Hi,
Comments below,
> On 19/01/2019, at 2:32 PM, Klemens Nanni wrote:
>
> A few assorted nits for consistency and proper format, no object change.
>
> OK?
>
> Index: pfctl.c
> ===
> RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
> retrieving revision 1.365
> diff -u -p -r1.365 pfctl.c
> --- pfctl.c 11 Jan 2019 03:09:24 - 1.365
> +++ pfctl.c 19 Jan 2019 01:29:20 -
> @@ -1485,7 +1485,6 @@ pfctl_load_ruleset(struct pfctl *pf, cha
> }
> } else if (pf->opts & PF_OPT_VERBOSE)
> printf("\n");
> -
> }
>
> if (pf->optimize)
> @@ -1851,7 +1850,6 @@ pfctl_set_limit(struct pfctl *pf, const
> {
> int i;
>
> -
> for (i = 0; pf_limits[i].name; i++) {
> if (strcasecmp(opt, pf_limits[i].name) == 0) {
> pf->limit[pf_limits[i].index] = limit;
> @@ -2217,7 +2215,7 @@ pfctl_show_anchors(int dev, int opts, ch
> err(1, "DIOCGETRULESET");
> if (!strcmp(pr.name, PF_RESERVED_ANCHOR))
> continue;
> - sub[0] = 0;
> + sub[0] = '\0';
> if (pr.path[0]) {
> strlcat(sub, pr.path, sizeof(sub));
> strlcat(sub, "/", sizeof(sub));
> @@ -2235,6 +2233,7 @@ const char *
> pfctl_lookup_option(char *cmd, const char **list)
> {
> const char *item = NULL;
> +
> if (cmd != NULL && *cmd)
> for (; *list; list++)
> if (!strncmp(cmd, *list, strlen(cmd))) {
> @@ -2580,15 +2579,15 @@ main(int argc, char *argv[])
> opts |= PF_OPT_SHOWALL;
> pfctl_load_fingerprints(dev, opts);
>
> - pfctl_show_rules(dev, path, opts, 0, anchorname,
> - 0, 0, -1);
> + pfctl_show_rules(dev, path, opts, PFCTL_SHOW_RULES,
> + anchorname, 0, 0, -1);
> pfctl_show_queues(dev, ifaceopt, opts,
> opts & PF_OPT_VERBOSE2);
> pfctl_show_states(dev, ifaceopt, opts, -1);
> pfctl_show_src_nodes(dev, opts);
> pfctl_show_status(dev, opts);
> - pfctl_show_rules(dev, path, opts, 1, anchorname,
> - 0, 0, -1);
> + pfctl_show_rules(dev, path, opts, PFCTL_SHOW_LABELS,
> + anchorname, 0, 0, -1);
> pfctl_show_timeouts(dev, opts);
> pfctl_show_limits(dev, opts);
> pfctl_show_tables(anchorname, opts);
> @@ -2671,7 +2670,7 @@ main(int argc, char *argv[])
> if (optiopt != NULL) {
> switch (*optiopt) {
> case 'n':
> - optimize = 0;
> + optimize = PF_OPTIMIZE_NONE;
> break;
> case 'b':
> optimize |= PF_OPTIMIZE_BASIC;
> Index: pfctl_parser.h
> ===
> RCS file: /cvs/src/sbin/pfctl/pfctl_parser.h,v
> retrieving revision 1.112
> diff -u -p -r1.112 pfctl_parser.h
> --- pfctl_parser.h6 Sep 2018 15:07:34 - 1.112
> +++ pfctl_parser.h19 Jan 2019 01:13:13 -
> @@ -57,6 +57,7 @@
> #define PF_NAT_PROXY_PORT_LOW 50001
> #define PF_NAT_PROXY_PORT_HIGH65535
>
> +#define PF_OPTIMIZE_NONE 0x
these PF_OPTIMIZE_* are bit-field definitions,
see e.g. pfctl_optimize.c:299.
But PF_OPTIMIZE_NONE is not, as pf->optimize & PF_OPTIMIZE_NONE
is never true, and pf->optimize |= PF_OPTIMIZE_NONE has no effect.
so I would leave this as optimize = 0; and drop PF_OPTIMIZE_NONE.
otherwise, ok procter
best,
Richard.
> #define PF_OPTIMIZE_BASIC 0x0001
> #define PF_OPTIMIZE_PROFILE 0x0002
>
>
Re: [PATCH] systat reports inaccurate statistics for disk i/o speed
On Sat, Jan 19, 2019 at 09:30:12PM +0900, Bryan Linton wrote:
> Hello tech@,
>
> I'd appreciate it if someone could review both this patch and my
> analysis.
>
>
> PROBLEM
> ---
> When running systat, the vmstat page shows inaccurate numbers for
> disk i/o speed (likely only on multiprocessor systems) when
> compared to both the iostat page, as well as the statistics
> reported by dd.
>
> To test this, open two xterms. Run "systat" in one, and
> "systat iostat" in the other. Then run something like,
> "dd if=/dev/zero of=zero.deleteme bs=64k" (or anything that will
> generate a lot of disk i/o).
>
> Observe that "systat" shows a different number for the reported
> i/o speed et. al., whereas "systat iostat" and pressing ^T while
> dd is running both return numbers in agreement with each other.
>
> On my 4-core system, the numbers returned by "systat" for "seeks",
> "xfers", and "speed" all appear to be exactly 4-times lower than
> are indicated in "systat iostat" as well as the transfer rate
> reported by dd.
>
>
> TENTATIVE ANALYSIS
> __
> (I am not an expert on OpenBSD's internals, and did not dig all
> the way into the depths of the kernel, so some of this analysis is
> based on conjecture).
>
> In /usr/src/usr.bin/systat/vmstat.c on line 349, (via some
> printf() debugging) the variable "etime" appears to be set to the
> sum total of time elapsed on ALL CPUs in the system.
>
> etime = 0;
> for (i = 0; i < CPUSTATES; i++) {
> X(cpustats.cs_time);
> etime += s.cpustats.cs_time[i];
> }
>
> This causes an error in calculations later on when it is used to
> calculate disk i/o statistics in vmstat.c (but not in iostat).
>
> vmstat.c:
>
> /* # of K transferred */
> words = (cur.dk_rbytes[dn] + cur.dk_wbytes[dn]) / 1024.0;
>
> putint((int)((float)cur.dk_seek[dn]/etime+0.5), DISKROW + 1, c, 5);
> putint((int)((float)(cur.dk_rxfer[dn] + cur.dk_wxfer[dn])/etime+0.5),
> DISKROW + 2, c, 5);
> putintmk((int)(words/etime + 0.5), DISKROW + 3, c, 5);
> putfloat(atime/etime, DISKROW + 4, c, 5, 1, 1);
>
> Compare the above to iostat.c, where
> etime = naptime;
> is set on line 143 before showtotal() prints the values.
>
> for (dn = 0; dn < cur.dk_ndrive; dn++) {
> rsum += cur.dk_rbytes[dn] / etime;
> wsum += cur.dk_wbytes[dn] / etime;
> rtsum += cur.dk_rxfer[dn] / etime;
> wtsum += cur.dk_wxfer[dn] / etime;
> mssum += ATIME(cur.dk_time, dn) / etime;
> }
>
> print_fld_str(FLD_IO_DEVICE, "Totals");
> print_fld_size(FLD_IO_READ, rsum);
> print_fld_size(FLD_IO_WRITE, wsum);
> print_fld_size(FLD_IO_RTPS, rtsum);
> print_fld_size(FLD_IO_WTPS, wtsum);
> print_fld_float(FLD_IO_SEC, mssum, 1);
>
>
> SOLUTION
>
>
> Set "etime = naptime;" in vmstat.c as well, as indicated in the
> attached patch.
>
>
> ADDENDUM
>
>
> I'm curious why iostat.c uses cur.dk_*[dn] directly whereas
> vmstat.c adds 0.5 to it.
> rsum += cur.dk_rbytes[dn] / etime;
rsum is a double. When adding a double to a double, there's no
rounding or truncation going on.
> vs.
> words = (cur.dk_rbytes[dn] + cur.dk_wbytes[dn]) / 1024.0;
> putintmk((int)(words/etime + 0.5), DISKROW + 3, c, 5);
>^^
When converting a double (or float) to an int, truncation happens. So
add 0.5 to turn it into rounding.
>
> Also, there are several casts in lines like
> putint((int)((float)cur.dk_seek[dn]/etime+0.5), DISKROW + 1, c, 5)
>
> I understand why the value would need to be cast to an (int),
> since putint() is expecting an int, but since etime is already a
> (double), wouldn't the C language's automatic type-promotion rules
> automatically promote everything to a (double) anyway?
Those casts are indeed unneeded,
-Otto
>
> Casting to a (float) would only seem to reduce precision, and then
> since it's immediately being cast to an (int) anyway, is the
> (float) cast really necessary? Removing it seems to cause no
> immediately noticeable ill effects on my system.
>
>
> I would appreciate any advice or explanation anyone can give regarding
> the above.
>
> Thank you.
>
> --
> Bryan
>
> Index: vmstat.c
> ===
> RCS file: /cvs/src/usr.bin/systat/vmstat.c,v
> retrieving revision 1.89
> diff -u -r1.89 vmstat.c
> --- vmstat.c 17 Nov 2018 23:10:08 - 1.89
> +++ vmstat.c 19 Jan 2019 11:37:33 -
> @@ -678,6 +678,8 @@
> {
> double words, atime;
>
> + etime = naptime;
> +
> c += DISKCOL;
>
> /* time busy in disk activity */
[PATCH] systat reports inaccurate statistics for disk i/o speed
Hello tech@,
I'd appreciate it if someone could review both this patch and my
analysis.
PROBLEM
---
When running systat, the vmstat page shows inaccurate numbers for
disk i/o speed (likely only on multiprocessor systems) when
compared to both the iostat page, as well as the statistics
reported by dd.
To test this, open two xterms. Run "systat" in one, and
"systat iostat" in the other. Then run something like,
"dd if=/dev/zero of=zero.deleteme bs=64k" (or anything that will
generate a lot of disk i/o).
Observe that "systat" shows a different number for the reported
i/o speed et. al., whereas "systat iostat" and pressing ^T while
dd is running both return numbers in agreement with each other.
On my 4-core system, the numbers returned by "systat" for "seeks",
"xfers", and "speed" all appear to be exactly 4-times lower than
are indicated in "systat iostat" as well as the transfer rate
reported by dd.
TENTATIVE ANALYSIS
__
(I am not an expert on OpenBSD's internals, and did not dig all
the way into the depths of the kernel, so some of this analysis is
based on conjecture).
In /usr/src/usr.bin/systat/vmstat.c on line 349, (via some
printf() debugging) the variable "etime" appears to be set to the
sum total of time elapsed on ALL CPUs in the system.
etime = 0;
for (i = 0; i < CPUSTATES; i++) {
X(cpustats.cs_time);
etime += s.cpustats.cs_time[i];
}
This causes an error in calculations later on when it is used to
calculate disk i/o statistics in vmstat.c (but not in iostat).
vmstat.c:
/* # of K transferred */
words = (cur.dk_rbytes[dn] + cur.dk_wbytes[dn]) / 1024.0;
putint((int)((float)cur.dk_seek[dn]/etime+0.5), DISKROW + 1, c, 5);
putint((int)((float)(cur.dk_rxfer[dn] + cur.dk_wxfer[dn])/etime+0.5),
DISKROW + 2, c, 5);
putintmk((int)(words/etime + 0.5), DISKROW + 3, c, 5);
putfloat(atime/etime, DISKROW + 4, c, 5, 1, 1);
Compare the above to iostat.c, where
etime = naptime;
is set on line 143 before showtotal() prints the values.
for (dn = 0; dn < cur.dk_ndrive; dn++) {
rsum += cur.dk_rbytes[dn] / etime;
wsum += cur.dk_wbytes[dn] / etime;
rtsum += cur.dk_rxfer[dn] / etime;
wtsum += cur.dk_wxfer[dn] / etime;
mssum += ATIME(cur.dk_time, dn) / etime;
}
print_fld_str(FLD_IO_DEVICE, "Totals");
print_fld_size(FLD_IO_READ, rsum);
print_fld_size(FLD_IO_WRITE, wsum);
print_fld_size(FLD_IO_RTPS, rtsum);
print_fld_size(FLD_IO_WTPS, wtsum);
print_fld_float(FLD_IO_SEC, mssum, 1);
SOLUTION
Set "etime = naptime;" in vmstat.c as well, as indicated in the
attached patch.
ADDENDUM
I'm curious why iostat.c uses cur.dk_*[dn] directly whereas
vmstat.c adds 0.5 to it.
rsum += cur.dk_rbytes[dn] / etime;
vs.
words = (cur.dk_rbytes[dn] + cur.dk_wbytes[dn]) / 1024.0;
putintmk((int)(words/etime + 0.5), DISKROW + 3, c, 5);
^^
Also, there are several casts in lines like
putint((int)((float)cur.dk_seek[dn]/etime+0.5), DISKROW + 1, c, 5)
I understand why the value would need to be cast to an (int),
since putint() is expecting an int, but since etime is already a
(double), wouldn't the C language's automatic type-promotion rules
automatically promote everything to a (double) anyway?
Casting to a (float) would only seem to reduce precision, and then
since it's immediately being cast to an (int) anyway, is the
(float) cast really necessary? Removing it seems to cause no
immediately noticeable ill effects on my system.
I would appreciate any advice or explanation anyone can give regarding
the above.
Thank you.
--
Bryan
Index: vmstat.c
===
RCS file: /cvs/src/usr.bin/systat/vmstat.c,v
retrieving revision 1.89
diff -u -r1.89 vmstat.c
--- vmstat.c17 Nov 2018 23:10:08 - 1.89
+++ vmstat.c19 Jan 2019 11:37:33 -
@@ -678,6 +678,8 @@
{
double words, atime;
+ etime = naptime;
+
c += DISKCOL;
/* time busy in disk activity */
Re: pfctl: use mnemonic macros, terminate string with null char
On Sat, Jan 19, 2019 at 05:14:56PM +1300, Richard Procter wrote: > > +#define PF_OPTIMIZE_NONE 0x > > these PF_OPTIMIZE_* are bit-field definitions, > see e.g. pfctl_optimize.c:299. While I'm aware of this, > But PF_OPTIMIZE_NONE is not, as pf->optimize & PF_OPTIMIZE_NONE > is never true, and pf->optimize |= PF_OPTIMIZE_NONE has no effect. I did not really consider such usage, which is indeed a bit dangerous. > so I would leave this as optimize = 0; and drop PF_OPTIMIZE_NONE. Agreed, thanks.
